Bug 188641 – [WHLSL] Inlining should be optional

• Component: WebGPU
• Status: RESOLVED FIXED

[Myles C. Maxfield]

We should't require inlining. It should be a step that can be selectively applied, or not, and have no affect on correctness. Different code generation backends may or may not wish to invoke inlining in certain cases.

[Thomas Denney]

Created attachment 349039 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 349039 [details]
Patch

Can we add a test that enables inlining for a couple functions to make sure it doesn't break?

[Myles C. Maxfield]

visitCallExpression(node) in Evaluator has a comment:

        // We evaluate inlined ASTs, so this can only be a native call.

This seems to be false now.

Also callFunction() still calls resolveInlinedFunction(), which seems either wrong or poorly named.

[Myles C. Maxfield]

Why does resolveInlinedFunction() call _inlineFunction()?

[Thomas Denney]

Created attachment 349352 [details]
Patch

[Thomas Denney]

*** Bug 189326 has been marked as a duplicate of this bug. ***

[Myles C. Maxfield]

Comment on attachment 349352 [details]
Patch

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

> Tools/WebGPUShadingLanguageRI/CallFunction.js:31
> +    let overload = program.globalNameContext.resolveFuncOverload(name, argumentTypes, null, true);

Cool.

> Tools/WebGPUShadingLanguageRI/Evaluator.js:40
> +        // This only occurs when a function returns void.

Comments should state "why," not "what." I recommend deleting this.

> Tools/WebGPUShadingLanguageRI/Inline.js:39
>      func.rewrite(new Inliner(program, func, visiting));

We should make sure we never inline entry points. Do we have a test for that?

> Tools/WebGPUShadingLanguageRI/Test.js:7646
> +tests.evaluationOrderForArguments = () => {

Can we add some tests that do some inlining?

[Thomas Denney]

Created attachment 349357 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 349357 [details]
Patch

Clearing flags on attachment: 349357

Committed r235879: <https://trac.webkit.org/changeset/235879>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44326053>

[Myles C. Maxfield]

Comment on attachment 349357 [details]
Patch

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

> Tools/WebGPUShadingLanguageRI/LateCheckAndLayoutBuffers.js:36
> +    for (let funcList of program.functions.values()) {
> +        for (let func of funcList) {
> +            if (func.isNative)
> +                continue;
> +            func.visit(new LateChecker());
> +            func.visit(new EBufferBuilder());
> +        }
> +    }

Why do we have to alternate between LateChecker() and EBufferBuilder()? Why can't we separate these out into two separate calls in Prepare()?

[Myles C. Maxfield]

Migrated to https://github.com/gpuweb/WHLSL/issues/98