Bug 198426 – [WHLSL] Auto initialize local variables

• Component: WebGPU
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

Created attachment 371069 [details]
patch

[Saam Barati]

Comment on attachment 371069 [details]
patch

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

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLAutoInitializeVariables.cpp:66
> +        StringPrintStream printStream;
> +        printStream.print(TypeDumper(*type));

Is this overkill just to get a nice function name? I could make this happen just on debug builds, and use some constant name for release builds.

[Myles C. Maxfield]

Comment on attachment 371069 [details]
patch

Please see my comments in https://bugs.webkit.org/show_bug.cgi?id=195771. Should that bug be marked as a duplicate of this one?

[Saam Barati]

(In reply to Myles C. Maxfield from comment #5)
> Comment on attachment 371047 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371047&action=review
> 
> Does the Metal compiler still produce warnings now?
> GPURenderPipelineMetal.mm has a FIXME with a link to this bug.
> 
> > Source/WebCore/Modules/webgpu/WHLSL/WHLSLAutoInitializeVariables.cpp:54
> > +        // Skip argument declarations.
> 
> Cool.
> 
> > Source/WebCore/Modules/webgpu/WHLSL/WHLSLAutoInitializeVariables.cpp:66
> > +        printStream.print(TypeDumper(*type));
> 
> 🤔 This seems unwise, though I do understand how it isn't observable. Are
> you confident this is a good idea?
I will make this debug builds only.

> 
> > Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:-360
> > -        auto* overload = resolveFunctionOverloadImpl(*getterFuncs, argumentTypeReferences, nullptr);
> 
> Excellent.
> 
> > Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLVariableDeclaration.h:73
> > +    void setInitializer(UniqueRef<Expression> expression)
> 
> Doesn't this need to take an rvalue reference? UniqueRef is move-only (I
> think???)
Rvalue references are just a cast. What happens here is a caller must move the argument into this value, calling the move constructor.

> 
> > Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:453
> >          m_stringBuilder.append(makeString(m_typeNamer.mangledNameForType(*variableDeclaration.type()), ' ', variableName, ";\n"));
> 
> Is this dead code now?
No. Since there are passes that run after this that create variables without initializers.

> 
> > Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:110
> > +            stringBuilder.append(makeString("    for (size_t i = 0; i < sizeof(", metalReturnName, "); ++i) {\n"));
> > +            stringBuilder.append("        ptr[i] = 0;\n");
> > +            stringBuilder.append("    }\n");
> 
> Can we remove the { } and pull it up into one line?

Sure!

[Saam Barati]

Created attachment 371767 [details]
patch for landing

[Saam Barati]

Created attachment 371768 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 371768 [details]
patch for landing

Clearing flags on attachment: 371768

Committed r246273: <https://trac.webkit.org/changeset/246273>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51593411>