Attachment 372501 Details for Bug 198988 – patch

[patch] patch

c-backup.diff (text/plain), 10.25 KB, created by Saam Barati on 2019-06-19 17:11:26 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2019-06-19 17:11:26 PDT

Size: 10.25 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 246615)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -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 NOBODY (OOPS!).
>+
>+        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  Jer Noble  <jer.noble@apple.com>
> 
>         iOS 12.2 Drawing portrait video to canvas is sideways
>Index: Source/WebCore/Modules/webgpu/WHLSL/WHLSLASTDumper.cpp
>===================================================================
>--- Source/WebCore/Modules/webgpu/WHLSL/WHLSLASTDumper.cpp	(revision 246613)
>+++ Source/WebCore/Modules/webgpu/WHLSL/WHLSLASTDumper.cpp	(working copy)
>@@ -639,7 +639,7 @@ void ASTDumper::visit(AST::ReadModifyWri
>     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: Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp
>===================================================================
>--- Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp	(revision 246613)
>+++ Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp	(working copy)
>@@ -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::ReadModifyWrite
>     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::VariableReferen
>     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));
> }
> 
>Index: Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLVariableDeclaration.h
>===================================================================
>--- Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLVariableDeclaration.h	(revision 246613)
>+++ Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLVariableDeclaration.h	(working copy)
>@@ -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: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 246613)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -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 NOBODY (OOPS!).
>+
>+        * 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  Jer Noble  <jer.noble@apple.com>
> 
>         iOS 12.2 Drawing portrait video to canvas is sideways
>Index: LayoutTests/webgpu/whlsl-checker-should-set-type-of-read-modify-write-variables-expected.txt
>===================================================================
>--- LayoutTests/webgpu/whlsl-checker-should-set-type-of-read-modify-write-variables-expected.txt	(nonexistent)
>+++ LayoutTests/webgpu/whlsl-checker-should-set-type-of-read-modify-write-variables-expected.txt	(working copy)
>@@ -0,0 +1,5 @@
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+PASS resultsFloat32Array[0] is 42
>+
>Index: LayoutTests/webgpu/whlsl-checker-should-set-type-of-read-modify-write-variables.html
>===================================================================
>--- LayoutTests/webgpu/whlsl-checker-should-set-type-of-read-modify-write-variables.html	(nonexistent)
>+++ LayoutTests/webgpu/whlsl-checker-should-set-type-of-read-modify-write-variables.html	(working copy)
>@@ -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>

Flags:

dino: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 198988: 372501 | 372510