[webkit-reviews] review granted: [Bug 200748] [WHLSL] Make "operator cast" constructors native : [Attachment 376571] 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 200748: [WHLSL] Make "operator cast" constructors native
https://bugs.webkit.org/show_bug.cgi?id=200748

Attachment 376571: patch

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




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

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

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:140
> +	   return
WTF::get<AST::ConstantExpression>(matrixType.typeArguments()[typeArgumentIndex]
).integerLiteral().value();

Can we move this to Intrinsics? I don't think this is the only place where this
kind of computation is performed.

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:211
> +	       if (nativeFunctionDeclaration.parameters().size() == numRows) {
> +		   // operator matrixMxN (vectorN, ..., vectorN)
> +		   for (unsigned i = 0; i < numRows; ++i) {
> +		       for (unsigned j = 0; j < numColumns; ++j)
> +			   stringBuilder.flexibleAppend("x[", j * numRows + i,
"] = ", args[i], "[", j, "];\n");
> +		   }
> +
> +	       } else {
> +		   // operator matrixMxN (scalar, ..., scalar)
> +		   unsigned index = 0;
> +		   for (unsigned i = 0; i < numRows; ++i) {
> +		       for (unsigned j = 0; j < numColumns; ++j) {
> +			   stringBuilder.flexibleAppend("x[", j * numRows + i,
"] = ", args[index], ";\n");
> +			   ++index;
> +		       }
> +		   }
> +	       }

Is there reason you're not just calling the MSL constructors because MSL and
HLSL define their matrices with different major-ness? This is probably worth a
comment.

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:327
> +	   auto& result =
downcast<AST::NativeTypeDeclaration>(downcast<AST::TypeReference>(downcast<AST:
:TypeDefinition>(typeReference.resolvedType()).type()).resolvedType());

Can this be shared with the lambda above?