Bug 200748 – [WHLSL] Make "operator cast" constructors native

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Saam Barati]

...

[Saam Barati]

patch forthcoming

[Saam Barati]

Created attachment 376571 [details]
patch

[EWS Watchlist]

Attachment 376571 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:169:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Comment on attachment 376571 [details]
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?

[Saam Barati]

Comment on attachment 376571 [details]
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.

We should put it on NativeTypeDeclaration.

>> 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?

it could, but I don't think having more functions in this limited scenario is gonna make anything more clear.

[Saam Barati]

Created attachment 376586 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 376586 [details]
patch for landing

Clearing flags on attachment: 376586

Committed r248814: <https://trac.webkit.org/changeset/248814>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54419405>