Bug 198163

And arrays

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

Created attachment 370830 [details] Test

Created attachment 370874 [details] WIP

Created attachment 370912 [details] WIP

<rdar://problem/51288820>

Created attachment 371158 [details] WIP

This should also test .length

Created attachment 371243 [details] WIP

Created attachment 371373 [details] WIP

Created attachment 371374 [details] Test

Created attachment 371375 [details] Test

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.

Created attachment 371471 [details] Patch

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

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

Trying to reproduce the test failure...

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

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?

Created attachment 371555 [details] Patch

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.

Created attachment 371900 [details] Patch for committing

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

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

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.

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

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.