Bug 198163 – [WHLSL] Implement array references

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Myles C. Maxfield]

Implement array references

[Myles C. Maxfield]

And arrays

[Myles C. Maxfield]

-    bool isReferenceType() const override { return false; }
+    bool isReferenceType() const override { return true; }

[Myles C. Maxfield]

Created attachment 370830 [details]
Test

[Myles C. Maxfield]

Created attachment 370874 [details]
WIP

[Myles C. Maxfield]

Created attachment 370912 [details]
WIP

[Radar WebKit Bug Importer]

<rdar://problem/51288820>

[Myles C. Maxfield]

Created attachment 371158 [details]
WIP

[Myles C. Maxfield]

This should also test .length

[Myles C. Maxfield]

Created attachment 371243 [details]
WIP

[Myles C. Maxfield]

Created attachment 371373 [details]
WIP

[Myles C. Maxfield]

Created attachment 371374 [details]
Test

[Myles C. Maxfield]

Created attachment 371375 [details]
Test

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

Created attachment 371471 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Myles C. Maxfield]

Trying to reproduce the test failure...

[Myles C. Maxfield]

That's interesting. The test failure doesn't use WHLSL.

[Justin Fan]

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?

[Myles C. Maxfield]

Created attachment 371555 [details]
Patch

[Saam Barati]

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.

[Myles C. Maxfield]

Created attachment 371900 [details]
Patch for committing

[Robin Morisset]

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.

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

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 :(

[Myles C. Maxfield]

Committed r246394: <https://trac.webkit.org/changeset/246394>

[Truitt Savell]

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.

[Truitt Savell]

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

[Myles C. Maxfield]

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.