[webkit-reviews] review granted: [Bug 193531] [WHLSL] Implement Metal code generation : [Attachment 359647] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Jan 20 17:21:54 PST 2019
Dean Jackson <dino at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 193531: [WHLSL] Implement Metal code generation
https://bugs.webkit.org/show_bug.cgi?id=193531
Attachment 359647: Patch
https://bugs.webkit.org/attachment.cgi?id=359647&action=review
--- Comment #15 from Dean Jackson <dino at apple.com> ---
Comment on attachment 359647
--> https://bugs.webkit.org/attachment.cgi?id=359647
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(*logicalExpres
sion.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.
More information about the webkit-reviews
mailing list