[webkit-reviews] review granted: [Bug 198890] [WHLSL] Make .length work : [Attachment 372267] patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Saam Barati
<sbarati at apple.com>'s request for review:
Bug 198890: [WHLSL] Make .length work
https://bugs.webkit.org/show_bug.cgi?id=198890

Attachment 372267: patch

https://bugs.webkit.org/attachment.cgi?id=372267&action=review




--- Comment #3 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 372267
  --> https://bugs.webkit.org/attachment.cgi?id=372267
patch

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

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLEntryPointScaffolding.cpp:318
> -		   stringBuilder.append(makeString("size_t ",
lengthTemporaryName, " = ", variableName, '.', lengthElementName, ".x;\n"));
> +		   stringBuilder.append(makeString("size_t ",
lengthTemporaryName, " = ", variableName, '.', lengthElementName, ".y;\n"));

👍

> LayoutTests/webgpu/whlsl-buffer-length.html:10
> +compute void computeShader(device float[] buffer : register(u0), float3
threadID : SV_DispatchThreadID) {

Can we turn the buffer into a uint[] buffer? That way no casts are needed.

Your "numthreads" directive above means that this code is racey. Instead you
should just say numthreads(1, 1, 1) and change line 56 below to say (1, 1, 1).

> LayoutTests/webgpu/whlsl-buffer-length.html:11
> +    buffer[uint(threadID.x)] = buffer[uint(threadID.x)] * 2.0;

What's the purpose of this line?

> LayoutTests/webgpu/whlsl-buffer-length.html:40
> +    for (let i = 0; i < 1337; ++i) {
> +	   bufferFloat32Array[i] = i + 1;
> +    }

Is there a need to fill the whole buffer? Can't you just fill the first item
and let it be overridden by the shader?