Bug 193531 – [WHLSL] Implement Metal code generation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[WHLSL] Implement Metal code generation

[Myles C. Maxfield]

Created attachment 359362 [details]
WIP

[Myles C. Maxfield]

Created attachment 359401 [details]
WIP

[Myles C. Maxfield]

Created attachment 359459 [details]
WIP

[Myles C. Maxfield]

Created attachment 359463 [details]
WIP

[Myles C. Maxfield]

Created attachment 359473 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:513:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 14 files


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

[Myles C. Maxfield]

(In reply to Myles C. Maxfield from comment #5)
> Created attachment 359473 [details]
> Patch

This patch has some basic packing & unpacking logic.

[Myles C. Maxfield]

Created attachment 359643 [details]
WIP

[Myles C. Maxfield]

Created attachment 359644 [details]
WIP

[Myles C. Maxfield]

Created attachment 359646 [details]
Patch

[Myles C. Maxfield]

Created attachment 359647 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 359647 [details]
Patch

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

> Source/WebCore/ChangeLog:44
> +              standard library, texture functions, and HLSL's half <-> int functions.

s/library/library)/

[Myles C. Maxfield]

Comment on attachment 359647 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        on it's own, though.

s/it's/its/

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:208
> +        m_stringBuilder.append("[[clang::fallthrough]];\n"); // FIXME: Make sure this is okay. Alternatively, we could do thing and just return here instead.

s/thing/nothing/

[Myles C. Maxfield]

Comment on attachment 359647 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:141
> +            checkErrorAndVisit(functionDefinition.block());

ASSERT() the stack is empty

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:156
> +            checkErrorAndVisit(functionDefinition.block());

ASSERT() the stack is empty

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:227
> +            checkErrorAndVisit(*forLoop.increment());

make sure to call .takeLast()

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:249
> +                m_stringBuilder.append(m_entryPointScaffolding->pack(variableName));

Make sure to call m_stack.takeLast() here

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:295
> +        auto name = ([](String input) -> String {
> +            if (input == "Add")
> +                return "fetch_add"_str;
> +            if (input == "And")
> +                return "fetch_and"_str;
> +            if (input == "Exchange")
> +                return "exchange"_str;
> +            if (input == "Max")
> +                return "fetch_max"_str;
> +            if (input == "Min")
> +                return "fetch_min"_str;
> +            if (input == "Or")
> +                return "fetch_or"_str;
> +            ASSERT(input == "Xor");
> +            return "fetch_xor"_str;
> +        })(nativeFunctionDeclaration.name().substring("Interlocked"_str.length()));

Consider promoting this to a real function

[Dean Jackson]

Comment on attachment 359647 [details]
Patch

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

This is mostly a rubber-stamp for such a huge patch :(

> Source/WebCore/ChangeLog:4
> +        [WHLSL] Implement Metal code generation
> +        https://bugs.webkit.org/show_bug.cgi?id=193531

It's a shame we haven't got any tests for this. I hope the entry point is behind ENABLE(WEBGPU).

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:139
> +            m_stringBuilder.append(makeString(m_entryPointScaffolding->signature(), " {"));

I wonder if it is better to
.append(m_entryPointsScaffolding->signature());
.append(" {");
since you already have the string builder.

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:197
> +        m_stringBuilder.append(makeString("if (!", m_stack.takeLast(), ") break;\n"));
> +        m_stringBuilder.append(makeString("} while(true);\n"));

Why not use the while condition?

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:223
> +        m_stringBuilder.append("for ( ; ; ) {\n");
> +        if (forLoop.condition()) {
> +            checkErrorAndVisit(*forLoop.condition());
> +            m_stringBuilder.append(makeString("if (!", m_stack.takeLast(), ") break;\n"));

Same here. I assume because any variables declared in the first statement inside for() are given unique names.

And another q about whether multiple calls to append are better than makestring.

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:461
> +        m_stringBuilder.append(makeString(m_typeNamer.mangledNameForType(*logicalExpression.resolvedType()), ' ', variableName, " = ", left, ' '));

Get rid of that last ' ', use " && "/" || ", and remove the space before right?

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLMetalCodeGenerator.cpp:44
> +    stringBuilder.append("// This was generated by WebKit's WHLSL -> MSL code generator. Do not edit!\n");

I don't think this is necessary.

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:105
> +    if (nativeFunctionDeclaration.name().startsWith("operator."_str)) {
> +        if (nativeFunctionDeclaration.name().endsWith("="_str)) {

I think using _s is slightly better here (but I'm not sure).

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLTypeNamer.cpp:167
> +};
> +
> +}
> +
> +}
> +
> +}
> +
> +#define SPECIALIZE_TYPE_TRAITS_WHLSL_BASE_TYPE_NAMED_NODE(ToValueTypeName, predicate) \
> +SPECIALIZE_TYPE_TRAITS_BEGIN(WebCore::WHLSL::Metal::ToValueTypeName) \
> +    static bool isType(const WebCore::WHLSL::Metal::BaseTypeNameNode& type) { return type.predicate; } \
> +SPECIALIZE_TYPE_TRAITS_END()
> +
> +SPECIALIZE_TYPE_TRAITS_WHLSL_BASE_TYPE_NAMED_NODE(ArrayTypeNameNode, isArrayTypeNameNode())
> +
> +SPECIALIZE_TYPE_TRAITS_WHLSL_BASE_TYPE_NAMED_NODE(ArrayReferenceTypeNameNode, isArrayReferenceTypeNameNode())
> +
> +SPECIALIZE_TYPE_TRAITS_WHLSL_BASE_TYPE_NAMED_NODE(PointerTypeNameNode, isPointerTypeNameNode())
> +
> +SPECIALIZE_TYPE_TRAITS_WHLSL_BASE_TYPE_NAMED_NODE(ReferenceTypeNameNode, isReferenceTypeNameNode())
> +
> +namespace WebCore {
> +
> +namespace WHLSL {
> +
> +namespace Metal {

I wonder if the macros should go at the end to avoid opening and closing three namespaces.

[Myles C. Maxfield]

Comment on attachment 359647 [details]
Patch

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

>> Source/WebCore/ChangeLog:4
>> +        https://bugs.webkit.org/show_bug.cgi?id=193531
> 
> It's a shame we haven't got any tests for this. I hope the entry point is behind ENABLE(WEBGPU).

The next patch will have enough for tests! 🎉🎉🎉

>> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:139
>> +            m_stringBuilder.append(makeString(m_entryPointScaffolding->signature(), " {"));
> 
> I wonder if it is better to
> .append(m_entryPointsScaffolding->signature());
> .append(" {");
> since you already have the string builder.

I had this originally, but I replaced it with this style. The addition of new lines of code made some of the more complicated parts of this patch super long (vertically) and difficult to understand. The makeString() calls for each line make it more readable.

>> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:197
>> +        m_stringBuilder.append(makeString("} while(true);\n"));
> 
> Why not use the while condition?

The ChangeLog explains this. Each condition turns into multiple expressions, which means it can't go inside a while condition.

>> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:105
>> +        if (nativeFunctionDeclaration.name().endsWith("="_str)) {
> 
> I think using _s is slightly better here (but I'm not sure).

It turns out that endsWith() supports string literals, so this should just be endsWith("=")

>> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLTypeNamer.cpp:167
>> +namespace Metal {
> 
> I wonder if the macros should go at the end to avoid opening and closing three namespaces.

They have to be defined before the is<>() and downcast<>() calls below.

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLTypeNamer.cpp:265
> +String TypeNamer::mangledNameForType(AST::NativeTypeDeclaration& nativeTypeDeclaration)

This should be in its own file.

[Myles C. Maxfield]

Committed r240230: <https://trac.webkit.org/changeset/240230>

[Radar WebKit Bug Importer]

<rdar://problem/47421993>

[Saam Barati]

Comment on attachment 359647 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:176
> +            checkErrorAndVisit(statement);

What would an error at this point be? Stack overflow?

>>> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:197
>>> +        m_stringBuilder.append(makeString("} while(true);\n"));
>> 
>> Why not use the while condition?
> 
> The ChangeLog explains this. Each condition turns into multiple expressions, which means it can't go inside a while condition.

Seems weird to internally generate a do-while then. Why not just make all loops consistently “for (;;)” or “while (true)”