Bug 188641

Summary: [WHLSL] Inlining should be optional
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: WebGPUAssignee: Thomas Denney <tdenney>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, tdenney, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 189484    
Bug Blocks: 176199, 188402, 189202    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Myles C. Maxfield 2018-08-15 23:00:14 PDT
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.
Comment 1 Thomas Denney 2018-09-06 10:17:31 PDT
Created attachment 349039 [details]
Patch
Comment 2 Myles C. Maxfield 2018-09-06 11:09:13 PDT
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?
Comment 3 Myles C. Maxfield 2018-09-06 12:54:07 PDT
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.
Comment 4 Myles C. Maxfield 2018-09-06 12:54:48 PDT
Why does resolveInlinedFunction() call _inlineFunction()?
Comment 5 Thomas Denney 2018-09-10 16:19:37 PDT
Created attachment 349352 [details]
Patch
Comment 6 Thomas Denney 2018-09-10 16:22:43 PDT
*** Bug 189326 has been marked as a duplicate of this bug. ***
Comment 7 Myles C. Maxfield 2018-09-10 16:35:47 PDT
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?
Comment 8 Thomas Denney 2018-09-10 17:01:34 PDT
Created attachment 349357 [details]
Patch
Comment 9 WebKit Commit Bot 2018-09-10 18:25:20 PDT
Comment on attachment 349357 [details]
Patch

Clearing flags on attachment: 349357

Committed r235879: <https://trac.webkit.org/changeset/235879>
Comment 10 WebKit Commit Bot 2018-09-10 18:25:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2018-09-10 18:26:37 PDT
<rdar://problem/44326053>
Comment 12 Myles C. Maxfield 2018-09-21 18:33:22 PDT
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()?
Comment 13 Myles C. Maxfield 2018-10-13 15:32:38 PDT
Migrated to https://github.com/gpuweb/WHLSL/issues/98