[webkit-reviews] review granted: [Bug 198704] [WHLSL] Hook up common texture functions : [Attachment 372470] 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 198704: [WHLSL] Hook up common texture functions
https://bugs.webkit.org/show_bug.cgi?id=198704

Attachment 372470: Patch

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




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

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

r=me

> Source/WebCore/ChangeLog:16
> +	   Reviewed by NOBODY (OOPS!).

I think you want this above the description above.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLCheckTextureReferences.cpp:112
> +    Visitor::visit(expression);
> +    Visitor::visit(expression.resolvedType());

Is this actually needed? How would an expression end up with this type without
us visiting that type in some other way?

If it is needed, I think you should be calling checkErrorAndVisit here instead.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLNameResolver.cpp:141
> +    if (newNameResolver.error())
> +	   setError();

Why not do it in the destructor instead of copy pasting it everywhere we
construct a nested name resolver?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLSynthesizeConstructors.cpp:85
> +    virtual ~FindAllTypes() = default;

👍🏼

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLSynthesizeConstructors.cpp:185
> +	       if (nativeTypeDeclaration.isTexture() ||
matches(nativeTypeDeclaration, program.intrinsics().samplerType()))

nit: You have this check in a few places throughout this patch. Maybe make a
helper method like "isTextureOrSampler" (with some better name)?

> Source/WebCore/platform/graphics/gpu/cocoa/GPUTextureMetal.mm:69
> +	   LOG(WebGPU, "%s: Texture cannot have OUTPUT_ATTACHMENT usage with
STORAGE or SAMPLED usages!", functionName);

Is this the kind of thing that is like an assert, or can user code trigger
this? Is it worth logging?

> LayoutTests/webgpu/whlsl-textures-getdimensions.html:14
> +    GetDimensions(theTexture, 0, &width, &height, &numberOfLevels);

I think it's worth addressing this ignored mipLevel in some way. Maybe either:
1. Removing it as a parameter in WHLSL.
2. Ensuring it's always a constant literal.
3. Filing a bug to do something above or something else in the future.