[WHLSL] The checker needs to resolve types for the anonymous variables in ReadModifyW...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 20 Jun 2019 02:05:28 +0000 (02:05 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 20 Jun 2019 02:05:28 +0000 (02:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=198988

Reviewed by Dean Jackson and Myles C. Maxfield.

Source/WebCore:

This patch makes it so that the Checker assigns types to the internal variables
in a read modify write expression. These were the only variables that didn't have
types ascribed to them.

This patch also does a fly by fix where we kept pointers to value types
in a HashMap in the checker. This is wrong precisely when the HashMap gets
resized. Instead, we now just store the value itself since we're just
dealing with a simple Variant that wraps either an empty struct or an
enum.

Test: webgpu/whlsl-checker-should-set-type-of-read-modify-write-variables.html

* Modules/webgpu/WHLSL/AST/WHLSLVariableDeclaration.h:
(WebCore::WHLSL::AST::VariableDeclaration::setType):
(WebCore::WHLSL::AST::VariableDeclaration::type const):
* Modules/webgpu/WHLSL/WHLSLASTDumper.cpp: Make it obvious that read
modify write expressions are such by prefixing them with "RMW".
(WebCore::WHLSL::ASTDumper::visit):
* Modules/webgpu/WHLSL/WHLSLChecker.cpp:
(WebCore::WHLSL::Checker::visit):

LayoutTests:

* webgpu/whlsl-checker-should-set-type-of-read-modify-write-variables-expected.txt: Added.
* webgpu/whlsl-checker-should-set-type-of-read-modify-write-variables.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@246625 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/webgpu/whlsl-checker-should-set-type-of-read-modify-write-variables-expected.txt [new file with mode: 0644]
LayoutTests/webgpu/whlsl-checker-should-set-type-of-read-modify-write-variables.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLVariableDeclaration.h
Source/WebCore/Modules/webgpu/WHLSL/WHLSLASTDumper.cpp
Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp

index c431591..5b88616 100644 (file)
@@ -1,3 +1,13 @@
+2019-06-19  Saam Barati  <sbarati@apple.com>
+
+        [WHLSL] The checker needs to resolve types for the anonymous variables in ReadModifyWrite expressions
+        https://bugs.webkit.org/show_bug.cgi?id=198988
+
+        Reviewed by Dean Jackson and Myles C. Maxfield.
+
+        * webgpu/whlsl-checker-should-set-type-of-read-modify-write-variables-expected.txt: Added.
+        * webgpu/whlsl-checker-should-set-type-of-read-modify-write-variables.html: Added.
+
 2019-06-19  Nikita Vasilyev  <nvasilyev@apple.com>
 
         REGRESSION(r240946): Web Inspector: Styles: Pasting multiple properties has issues
diff --git a/LayoutTests/webgpu/whlsl-checker-should-set-type-of-read-modify-write-variables-expected.txt b/LayoutTests/webgpu/whlsl-checker-should-set-type-of-read-modify-write-variables-expected.txt
new file mode 100644 (file)
index 0000000..4660202
--- /dev/null
@@ -0,0 +1,5 @@
+PASS successfullyParsed is true
+
+TEST COMPLETE
+PASS resultsFloat32Array[0] is 42
+
diff --git a/LayoutTests/webgpu/whlsl-checker-should-set-type-of-read-modify-write-variables.html b/LayoutTests/webgpu/whlsl-checker-should-set-type-of-read-modify-write-variables.html
new file mode 100644 (file)
index 0000000..464e268
--- /dev/null
@@ -0,0 +1,78 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../resources/js-test-pre.js"></script>
+</head>
+<body>
+<script>
+const shaderSource = `
+[numthreads(1, 1, 1)]
+compute void computeShader(device float[] buffer : register(u0), float3 threadID : SV_DispatchThreadID) {
+    float4 vec;
+    vec[0] += 42.0;
+    buffer[uint(threadID.x)] = vec[0];
+}
+`;
+let resultsFloat32Array;
+async function start() {
+    const adapter = await navigator.gpu.requestAdapter();
+    const device = await adapter.requestDevice();
+
+    const shaderModule = device.createShaderModule({code: shaderSource, isWHLSL: true});
+    const computeStage = {module: shaderModule, entryPoint: "computeShader"};
+
+    const bindGroupLayoutDescriptor = {bindings: [{binding: 0, visibility: 7, type: "storage-buffer"}]};
+    const bindGroupLayout = device.createBindGroupLayout(bindGroupLayoutDescriptor);
+    const pipelineLayoutDescriptor = {bindGroupLayouts: [bindGroupLayout]};
+    const pipelineLayout = device.createPipelineLayout(pipelineLayoutDescriptor);
+
+    const computePipelineDescriptor = {computeStage, layout: pipelineLayout};
+    const computePipeline = device.createComputePipeline(computePipelineDescriptor);
+
+    const size = Float32Array.BYTES_PER_ELEMENT * 1;
+
+    const bufferDescriptor = {size, usage: GPUBufferUsage.MAP_WRITE | GPUBufferUsage.TRANSFER_SRC};
+    const buffer = device.createBuffer(bufferDescriptor);
+    const bufferArrayBuffer = await buffer.mapWriteAsync();
+    const bufferFloat32Array = new Float32Array(bufferArrayBuffer);
+    bufferFloat32Array[0] = 0;
+    buffer.unmap();
+
+    const resultsBufferDescriptor = {size, usage: GPUBufferUsage.STORAGE | GPUBufferUsage.TRANSFER_DST | GPUBufferUsage.MAP_READ};
+    const resultsBuffer = device.createBuffer(resultsBufferDescriptor);
+
+    const bufferBinding = {buffer: resultsBuffer, size};
+    const bindGroupBinding = {binding: 0, resource: bufferBinding};
+    const bindGroupDescriptor = {layout: bindGroupLayout, bindings: [bindGroupBinding]};
+    const bindGroup = device.createBindGroup(bindGroupDescriptor);
+
+    const commandEncoder = device.createCommandEncoder(); // {}
+    commandEncoder.copyBufferToBuffer(buffer, 0, resultsBuffer, 0, size);
+    const computePassEncoder = commandEncoder.beginComputePass();
+    computePassEncoder.setPipeline(computePipeline);
+    computePassEncoder.setBindGroup(0, bindGroup);
+    computePassEncoder.dispatch(1, 1, 1);
+    computePassEncoder.endPass();
+    const commandBuffer = commandEncoder.finish();
+    device.getQueue().submit([commandBuffer]);
+
+    const resultsArrayBuffer = await resultsBuffer.mapReadAsync();
+    resultsFloat32Array = new Float32Array(resultsArrayBuffer);
+    shouldBe("resultsFloat32Array[0]", "42");
+    resultsBuffer.unmap();
+}
+if (window.testRunner)
+    testRunner.waitUntilDone();
+window.addEventListener("load", function() {
+    start().then(function() {
+        if (window.testRunner)
+            testRunner.notifyDone();
+    }, function() {
+        if (window.testRunner)
+            testRunner.notifyDone();
+    });
+});
+</script>
+<script src="../resources/js-test-post.js"></script>
+</body>
+</html>
index b7435ef..4861d5b 100644 (file)
@@ -1,3 +1,31 @@
+2019-06-19  Saam Barati  <sbarati@apple.com>
+
+        [WHLSL] The checker needs to resolve types for the anonymous variables in ReadModifyWrite expressions
+        https://bugs.webkit.org/show_bug.cgi?id=198988
+
+        Reviewed by Dean Jackson and Myles C. Maxfield.
+
+        This patch makes it so that the Checker assigns types to the internal variables
+        in a read modify write expression. These were the only variables that didn't have
+        types ascribed to them.
+
+        This patch also does a fly by fix where we kept pointers to value types
+        in a HashMap in the checker. This is wrong precisely when the HashMap gets
+        resized. Instead, we now just store the value itself since we're just
+        dealing with a simple Variant that wraps either an empty struct or an
+        enum.
+
+        Test: webgpu/whlsl-checker-should-set-type-of-read-modify-write-variables.html
+
+        * Modules/webgpu/WHLSL/AST/WHLSLVariableDeclaration.h:
+        (WebCore::WHLSL::AST::VariableDeclaration::setType):
+        (WebCore::WHLSL::AST::VariableDeclaration::type const):
+        * Modules/webgpu/WHLSL/WHLSLASTDumper.cpp: Make it obvious that read
+        modify write expressions are such by prefixing them with "RMW".
+        (WebCore::WHLSL::ASTDumper::visit):
+        * Modules/webgpu/WHLSL/WHLSLChecker.cpp:
+        (WebCore::WHLSL::Checker::visit):
+
 2019-06-19  Devin Rousso  <drousso@apple.com>
 
         Web Inspector: Network: replace CFNetwork SPI with new API where able
index 9f8d99b..37ea386 100644 (file)
@@ -64,7 +64,15 @@ public:
 
     String& name() { return m_name; }
 
-    const Optional<UniqueRef<UnnamedType>>& type() const { return m_type; } // Anonymous variables inside ReadModifyWriteExpressions have their type set by the type checker.
+    // We use this for ReadModifyWrite expressions, since we don't know the type of their
+    // internal variables until the checker runs. All other variables should start life out
+    // with a type.
+    void setType(UniqueRef<UnnamedType> type)
+    {
+        ASSERT(!m_type);
+        m_type = WTFMove(type);
+    }
+    const Optional<UniqueRef<UnnamedType>>& type() const { return m_type; }
     UnnamedType* type() { return m_type ? &*m_type : nullptr; }
     Optional<Semantic>& semantic() { return m_semantic; }
     Expression* initializer() { return m_initializer ? &*m_initializer : nullptr; }
index 2ebf862..5cfc8e2 100644 (file)
@@ -639,7 +639,7 @@ void ASTDumper::visit(AST::ReadModifyWriteExpression& readModifyWriteExpression)
     auto oldVariable = readModifyWriteExpression.oldVariableReference();
     auto newVariable = readModifyWriteExpression.newVariableReference();
 
-    m_out.print("(");
+    m_out.print("RMW(");
     visit(oldVariable.get());
     m_out.print(" = ");
     visit(readModifyWriteExpression.leftValue());
index d894b92..c05c733 100644 (file)
@@ -463,7 +463,7 @@ private:
     bool isBoolType(ResolvingType&);
     struct RecurseInfo {
         ResolvingType& resolvingType;
-        AST::TypeAnnotation& typeAnnotation;
+        const AST::TypeAnnotation typeAnnotation;
     };
     Optional<RecurseInfo> recurseAndGetInfo(AST::Expression&, bool requiresLeftValue = false);
     Optional<RecurseInfo> getInfo(AST::Expression&, bool requiresLeftValue = false);
@@ -859,13 +859,15 @@ void Checker::visit(AST::ReadModifyWriteExpression& readModifyWriteExpression)
     if (!leftValueInfo)
         return;
 
-    // FIXME: https://bugs.webkit.org/show_bug.cgi?id=198166 Figure out what to do with the ReadModifyWriteExpression's AnonymousVariables.
+    readModifyWriteExpression.oldValue().setType(leftValueInfo->resolvingType.getUnnamedType()->clone());
 
     auto newValueInfo = recurseAndGetInfo(readModifyWriteExpression.newValueExpression());
     if (!newValueInfo)
         return;
 
-    if (!matchAndCommit(leftValueInfo->resolvingType, newValueInfo->resolvingType)) {
+    if (Optional<UniqueRef<AST::UnnamedType>> matchedType = matchAndCommit(leftValueInfo->resolvingType, newValueInfo->resolvingType))
+        readModifyWriteExpression.newValue().setType(WTFMove(matchedType.value()));
+    else {
         setError();
         return;
     }
@@ -1131,8 +1133,7 @@ void Checker::visit(AST::VariableReference& variableReference)
     ASSERT(variableReference.variable()->type());
     
     AST::TypeAnnotation typeAnnotation = AST::RightValue();
-    if (!variableReference.variable()->isAnonymous()) // FIXME: https://bugs.webkit.org/show_bug.cgi?id=198166 This doesn't seem right.
-        typeAnnotation = AST::LeftValue { AST::AddressSpace::Thread };
+    typeAnnotation = AST::LeftValue { AST::AddressSpace::Thread };
     assignType(variableReference, variableReference.variable()->type()->clone(), WTFMove(typeAnnotation));
 }