[webkit-reviews] review granted: [Bug 198186] [WHLSL] Standard library is too big to directly include in WebCore : [Attachment 373343] Without standard library changes

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted  review:
Bug 198186: [WHLSL] Standard library is too big to directly include in WebCore
https://bugs.webkit.org/show_bug.cgi?id=198186

Attachment 373343: Without standard library changes

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




--- Comment #22 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 373343
  --> https://bugs.webkit.org/attachment.cgi?id=373343
Without standard library changes

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

r=me

> Source/WebCore/ChangeLog:29
> +	   form /* Functions named xyz */. At build time, a Python script looks
for all these comments, and builds a

Is this being done in the spec repo?

>
Source/WebCore/Modules/webgpu/WHLSL/Cocoa/WHLSLStandardLibraryUtilities.cpp:44
> +static Vector<LChar> decompressStandardLibrary()

Do we not just have a helper "gunzip" method in WebKit? If not, maybe we can
add a "gunzip" helper in WTF?

>
Source/WebCore/Modules/webgpu/WHLSL/Cocoa/WHLSLStandardLibraryUtilities.cpp:112
> +	   HashSet<String> result;
> +	   std::swap(result, m_functionNames);
> +	   return result;

style nit:
Why not: "return WTFMove(m_functionNames)"?

>
Source/WebCore/Modules/webgpu/WHLSL/Cocoa/WHLSLStandardLibraryUtilities.cpp:151
> +    functionNames.add("operator cast"_str);
> +    functionNames.add("ddx"_str);
> +    functionNames.add("ddy"_str);
> +    functionNames.add("DeviceMemoryBarrierWithGroupSync"_str);
> +    functionNames.add("GroupMemoryBarrierWithGroupSync"_str);
> +    functionNames.add("AllMemoryBarrierWithGroupSync"_str);

This means we always parse these? Why is that needed? Maybe a comment?

>
Source/WebCore/Modules/webgpu/WHLSL/Cocoa/WHLSLStandardLibraryUtilities.cpp:166
> +	   functionNames.clear();

style nit: this isn't necessary since you're assigning to it below.

>
Source/WebCore/Modules/webgpu/WHLSL/Cocoa/WHLSLStandardLibraryUtilities.cpp:177
> +}
> +
> +}

style nit: add namespace ending comments

>
Source/WebCore/Modules/webgpu/WHLSL/WHLSLBuildStandardLibraryFunctionMap.py:87
> +    outfile.write("	  result.add(\"" + previousName + "\"_str,
SubstringLocation { " + str(previous) + ", " + str(match.start()) + " });\n")
> +    previous = match.start()
> +    previousName = match.group(1)
> +outfile.write("    result.add(\"" + previousName + "\"_str,
SubstringLocation { " + str(previous) + ", " + str(len(contents)) + " });\n")

Can you open a bug and add a FIXME here about just computing the hash table at
compile time in the future? Kinda like we do with the static property hash
tables in JSC. We could probably just reuse that code.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:420
> +	   || functionDefinition.name() == "operator>>")

👍🏼

> LayoutTests/webgpu/whlsl-matrix.html:39
> +    foo = foo + float2x3(float3(1.0, 1.0, 1.0), float3(1.0, 1.0, 1.0));
>      if (!allEqual(foo, 1))
>	   return;
>  
> -    foo = foo * 8.5;
> +    foo = foo * float2x3(float3(8.5, 8.5, 8.5), float3(8.5, 8.5, 8.5));
>      if (!allEqual(foo, 8.5))
>	   return;
>  
> -    foo = foo - 7.5;
> +    foo = foo - float2x3(float3(7.5, 7.5, 7.5), float3(7.5, 7.5, 7.5));
>      if (!allEqual(foo, 1))
>	   return;
>  
> -    foo = foo + 1.0;
> +    foo = foo + float2x3(float3(1.0, 1.0, 1.0), float3(1.0, 1.0, 1.0));

Why? Isn't this part of the stdlib to do math with a single parameter against
all matrix elements?