[webkit-reviews] review granted: [Bug 198163] [WHLSL] Implement array references : [Attachment 371555] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jun 9 13:35:20 PDT 2019


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

Attachment 371555: Patch

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




--- Comment #21 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 371555
  --> https://bugs.webkit.org/attachment.cgi?id=371555
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.


More information about the webkit-reviews mailing list