[webkit-reviews] review granted: [Bug 198644] [WHLSL] Hook up compute : [Attachment 371612] Patch

[bugzilla-daemon at webkit.org]

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

Attachment 371612: Patch

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




--- Comment #6 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 371612
  --> https://bugs.webkit.org/attachment.cgi?id=371612
Patch

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

r=me

> Source/WebCore/ChangeLog:18
> +	   doesn't always match WHLSL's (and HLSL's type). For example, in
WHLSL and HLSL,        SV_DispatchThreadID variables have to be a float3, but
in Metal, they are a uint3.

Why do they have to be float3 in WHLSL?

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLEntryPointScaffolding.cpp:195
> +	   ASSERT(builtInSemantic.variable() ==
AST::BuiltInSemantic::Variable::SVGroupThreadID);

Nit: I like to write this out as a case, and omit the “default”. Then, when
anyone adds a new enum value, this becomes a compile error

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLComputeDimensions.cpp:54
> +	   Visitor::visit(functionDeclaration);

Why?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStandardLibrary.txt:596
> +native float operator.w(float4);/*

And this too?

> LayoutTests/webgpu/whlsl-compute.html:9
> +[numthreads(2, 1, 1)]

Out of curiosity, why do we have thread IDs in three dimensions? Is this just a
useful format for a lot of shaders to avoid doing some math or something?

> LayoutTests/webgpu/whlsl-compute.html:64
> +    shouldBe("resultsFloat32Array[0]", "2");

👌🏼