[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