Bug 198163 - [WHLSL] Implement array references
Summary: [WHLSL] Implement array references
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on: 198399
Blocks: 198644
  Show dependency treegraph
 
Reported: 2019-05-22 22:31 PDT by Myles C. Maxfield
Modified: 2019-06-18 00:57 PDT (History)
12 users (show)

See Also:


Attachments
Test (4.28 KB, text/html)
2019-05-28 23:02 PDT, Myles C. Maxfield
no flags Details
WIP (9.94 KB, patch)
2019-05-29 12:15 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (22.61 KB, patch)
2019-05-29 19:58 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (4.08 KB, patch)
2019-06-02 10:44 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (31.90 KB, patch)
2019-06-03 20:57 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (72.74 KB, patch)
2019-06-04 21:30 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Test (4.25 KB, text/html)
2019-06-04 21:36 PDT, Myles C. Maxfield
no flags Details
Test (4.26 KB, text/html)
2019-06-04 21:41 PDT, Myles C. Maxfield
no flags Details
Patch (95.33 KB, patch)
2019-06-05 21:38 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (3.17 MB, application/zip)
2019-06-05 22:54 PDT, Build Bot
no flags Details
Patch (103.47 KB, patch)
2019-06-06 21:00 PDT, Myles C. Maxfield
sbarati: review+
Details | Formatted Diff | Diff
Patch for committing (102.07 KB, patch)
2019-06-11 17:38 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2019-05-22 22:31:15 PDT
Implement array references
Comment 1 Myles C. Maxfield 2019-05-22 22:31:33 PDT
And arrays
Comment 2 Myles C. Maxfield 2019-05-28 23:01:12 PDT
-    bool isReferenceType() const override { return false; }
+    bool isReferenceType() const override { return true; }
Comment 3 Myles C. Maxfield 2019-05-28 23:02:05 PDT
Created attachment 370830 [details]
Test
Comment 4 Myles C. Maxfield 2019-05-29 12:15:21 PDT
Created attachment 370874 [details]
WIP
Comment 5 Myles C. Maxfield 2019-05-29 19:58:30 PDT
Created attachment 370912 [details]
WIP
Comment 6 Radar WebKit Bug Importer 2019-05-30 20:30:10 PDT
<rdar://problem/51288820>
Comment 7 Myles C. Maxfield 2019-06-02 10:44:07 PDT
Created attachment 371158 [details]
WIP
Comment 8 Myles C. Maxfield 2019-06-02 10:49:52 PDT
This should also test .length
Comment 9 Myles C. Maxfield 2019-06-03 20:57:56 PDT
Created attachment 371243 [details]
WIP
Comment 10 Myles C. Maxfield 2019-06-04 21:30:36 PDT
Created attachment 371373 [details]
WIP
Comment 11 Myles C. Maxfield 2019-06-04 21:36:14 PDT
Created attachment 371374 [details]
Test
Comment 12 Myles C. Maxfield 2019-06-04 21:41:41 PDT
Created attachment 371375 [details]
Test
Comment 13 Myles C. Maxfield 2019-06-05 20:01:57 PDT
Splitting the array piece into https://bugs.webkit.org/show_bug.cgi?id=198414 because arrays can't be resources, because you can't know at compile time whether the buffer will be long enough to satisfy the array.
Comment 14 Myles C. Maxfield 2019-06-05 21:38:22 PDT
Created attachment 371471 [details]
Patch
Comment 15 Build Bot 2019-06-05 22:54:04 PDT
Comment on attachment 371471 [details]
Patch

Attachment 371471 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12392249

New failing tests:
webgpu/buffer-resource-triangles.html
Comment 16 Build Bot 2019-06-05 22:54:06 PDT
Created attachment 371475 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 17 Myles C. Maxfield 2019-06-06 09:17:27 PDT
Trying to reproduce the test failure...
Comment 18 Myles C. Maxfield 2019-06-06 09:23:16 PDT
That's interesting. The test failure doesn't use WHLSL.
Comment 19 Justin Fan 2019-06-06 15:54:12 PDT
Comment on attachment 371471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=371471&action=review

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBindGroupLayoutMetal.mm:87
> +    [mtlArgument.get() setDataType:dataType];

Minor: don't need to call .get() if the receiver is a RetainPtr.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBindGroupLayoutMetal.mm:143
> +                RetainPtr<MTLArgumentDescriptor> mtlArgument = argumentDescriptor(MTLDataTypeUInt2, *extraIndex);

cool.

> LayoutTests/ChangeLog:9
> +        * webgpu/whlsl-buffer-fragment.html: Copied from LayoutTests/webgpu/whlsl-dont-crash-parsing-enum.html.

Is this correct?
Comment 20 Myles C. Maxfield 2019-06-06 21:00:44 PDT
Created attachment 371555 [details]
Patch
Comment 21 Saam Barati 2019-06-09 13:35:20 PDT
Comment on attachment 371555 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=371555&action=review

Nice. r=me with some super small nits.

> Source/WebCore/ChangeLog:12
> +        1. The Javascript compiler has a behavior where anders that are called with an array reference

super duper small nit: Javascript => JavaScript

> Source/WebCore/ChangeLog:14
> +           reference is already a reference type, so it's silly to operate on a pointer to a reference.

"pointer to a reference". Isn't that just a pointer then? FWIW, that doesn't seem silly to me.

> Source/WebCore/ChangeLog:24
> +        2. Creating a bind group from the WebGPU API has to retain information about buffer lengths for
> +           each buffer so the shader can properly perform bounds checks. This can be broken down into a
> +           few pieces:

So is the length of the buffer a constant for the WHLSL compiler?

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLEntryPointScaffolding.cpp:290
> +                stringBuilder.append(makeString("size_t ", lengthTemporaryName, " = ", variableName, '.', lengthElementName, ".x;\n"));
> +                stringBuilder.append(makeString(lengthTemporaryName, " = ", lengthTemporaryName, " << 32;\n"));
> +                stringBuilder.append(makeString(lengthTemporaryName, " = ", lengthTemporaryName, " | ", variableName, '.', lengthElementName, ".y;\n"));
> +                stringBuilder.append(makeString(lengthTemporaryName, " = ", lengthTemporaryName, " / sizeof(", mangledTypeName, ");\n"));

This is assuming size_t is 64-bit. Why not just use "uint64_t" instead? That feels safer.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:977
> +        return { makeUniqueRef<AST::PointerType>(Lexer::Token(namedType.origin()), addressSpace, AST::TypeReference::wrap(Lexer::Token(namedType.origin()), namedType)) };

nit: You don't need to wrap namedType.origin() in a call to copy ctor.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:992
> +    return { makeUniqueRef<AST::PointerType>(Lexer::Token(unnamedType.origin()), addressSpace, unnamedType.clone()) };

ditto

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:1018
> +        if (auto argumentTypeForAndOverload = WHLSL::argumentTypeForAndOverload(*baseUnnamedType, *leftAddressSpace)) {

Why WHLSL::? Aren't we already in that namespace?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:1035
> +    if (auto argumentTypeForAndOverload = WHLSL::argumentTypeForAndOverload(*baseUnnamedType, AST::AddressSpace::Thread)) {

ditto

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:86
> +enum class WhichAnder {

name nit: Maybe "AnderType"?

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBindGroupLayoutMetal.mm:122
> +            default:
> +                ASSERT(binding.type == GPUBindingType::DynamicStorageBuffer);

nit: I like to write out these cases, with no default. Then it becomes a compile error anytime someone adds a new enum value to this GPUBindingType enum class.
Comment 22 Myles C. Maxfield 2019-06-11 17:38:29 PDT
Created attachment 371900 [details]
Patch for committing
Comment 23 Robin Morisset 2019-06-12 17:04:11 PDT
Comment on attachment 371555 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=371555&action=review

>> Source/WebCore/ChangeLog:14
>> +           reference is already a reference type, so it's silly to operate on a pointer to a reference.
> 
> "pointer to a reference". Isn't that just a pointer then? FWIW, that doesn't seem silly to me.

I don't think it is just a pointer. An array reference in WHLSL is a fat pointer, basically a pointer + size for use in bounds checking. So we have an extra level of indirection here. Not ideal, but simplifies things.

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLPropertyAccessExpression.h:102
> +    UniqueRef<Expression>& baseReference() { return m_base; }

Is there a reason not to return Expression& ? Returning a reference to what is semantically another reference feels a bit weird.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:-120
> -static AST::NativeFunctionDeclaration resolveWithOperatorAnderIndexer(AST::CallExpression& callExpression, AST::ArrayReferenceType& firstArgument, const Intrinsics& intrinsics)

Nice simplification.
Comment 24 Myles C. Maxfield 2019-06-12 20:52:00 PDT
Comment on attachment 371555 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=371555&action=review

>>> Source/WebCore/ChangeLog:14
>>> +           reference is already a reference type, so it's silly to operate on a pointer to a reference.
>> 
>> "pointer to a reference". Isn't that just a pointer then? FWIW, that doesn't seem silly to me.
> 
> I don't think it is just a pointer. An array reference in WHLSL is a fat pointer, basically a pointer + size for use in bounds checking. So we have an extra level of indirection here. Not ideal, but simplifies things.

I mean "reference" to mean "reference type." Reference types are either A) pointers or B) array references. So if you have arrayReference.thingy and "thingy" has an ander, you don't want to have the signature for the ander to be thread Foo* operator.thingy(thread ArrayReference[]*) because "[]*" is silly when you've already got an array reference.

>> Source/WebCore/ChangeLog:24
>> +           few pieces:
> 
> So is the length of the buffer a constant for the WHLSL compiler?

Yes. It isn't exposed as a variable, though; it's inside the guts of the array reference. (In MSL, array references are represented as a pointer and a length, wrapped in a struct.)

>> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLEntryPointScaffolding.cpp:290
>> +                stringBuilder.append(makeString(lengthTemporaryName, " = ", lengthTemporaryName, " / sizeof(", mangledTypeName, ");\n"));
> 
> This is assuming size_t is 64-bit. Why not just use "uint64_t" instead? That feels safer.

uint64_t doesn't exist in Metal. size_t does.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:977
>> +        return { makeUniqueRef<AST::PointerType>(Lexer::Token(namedType.origin()), addressSpace, AST::TypeReference::wrap(Lexer::Token(namedType.origin()), namedType)) };
> 
> nit: You don't need to wrap namedType.origin() in a call to copy ctor.

Yep. I'll do this in a follow up, across the whole code base.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:1018
>> +        if (auto argumentTypeForAndOverload = WHLSL::argumentTypeForAndOverload(*baseUnnamedType, *leftAddressSpace)) {
> 
> Why WHLSL::? Aren't we already in that namespace?

The variable is named the same thing as the function, so I have to qualify the second one.

>> Source/WebCore/platform/graphics/gpu/cocoa/GPUBindGroupLayoutMetal.mm:122
>> +                ASSERT(binding.type == GPUBindingType::DynamicStorageBuffer);
> 
> nit: I like to write out these cases, with no default. Then it becomes a compile error anytime someone adds a new enum value to this GPUBindingType enum class.

I prefer that too, but every time I try to do it, one of the ports fails to build. I'll try to do it again.
Comment 25 Myles C. Maxfield 2019-06-12 20:53:55 PDT
Comment on attachment 371555 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=371555&action=review

>> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLPropertyAccessExpression.h:102
>> +    UniqueRef<Expression>& baseReference() { return m_base; }
> 
> Is there a reason not to return Expression& ? Returning a reference to what is semantically another reference feels a bit weird.

I have to move from m_base :(
Comment 26 Myles C. Maxfield 2019-06-12 22:38:32 PDT
Committed r246394: <https://trac.webkit.org/changeset/246394>
Comment 27 Truitt Savell 2019-06-13 08:19:06 PDT
Looks like the new test webgpu/whlsl-buffer-fragment.html

added in https://trac.webkit.org/changeset/246394/webkit

is failing on High Sierra WK2.

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=webgpu%2Fwhlsl-buffer-fragment.html

Diff:
https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/r246397%20(11712)/webgpu/whlsl-buffer-fragment-diffs.html


It is drawing a back square instead of a white one in the center of a blue square.
Comment 28 Truitt Savell 2019-06-17 10:04:08 PDT
This test is also failing webgpu/whlsl-buffer-vertex.html
It will have an image failure or a timeout. 

Hsitory:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=webgpu%2Fwhlsl-buffer-vertex.html
Comment 29 Myles C. Maxfield 2019-06-18 00:57:42 PDT
A reduced shader:

#include <metal_stdlib>

using namespace metal;

struct VertexInput {
    float4 theAttribute [[attribute(0)]];
};

struct VertexOutput {
    float4 thePosition [[position]];
};

vertex VertexOutput vertexShader(VertexInput vertexInput [[stage_in]]) {
    VertexOutput vertexOutput;
    vertexOutput.thePosition = vertexInput.theAttribute;
    return vertexOutput;
}

struct ArgumentBuffer {
    device float* floatBuffer [[id(0)]];
    uint2 length [[id(1)]];
};

fragment float4 fragmentShader(device ArgumentBuffer& argumentBuffer [[buffer(0)]]) {
    /*float scalar = 0;
    if (0 < argumentBuffer.length.x) scalar = argumentBuffer.floatBuffer[0];
    return float4(scalar, 1, 1, 1);*/

    device float* pointer = nullptr;
    if (0 < argumentBuffer.length.x) pointer = &(argumentBuffer.floatBuffer[0]);
    return float4(*pointer, 1, 1, 1);
}

This shader doesn't work. However, if you swap out the second block for the first block in the fragment shader, the shader works as expected.

It seems like conditionally assigning to pointers doesn't work on this particular hardware on this particular operating system.