Bug 188641 - [WHLSL] Inlining should be optional
Summary: [WHLSL] Inlining should be optional
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Thomas Denney
URL:
Keywords: InRadar
: 189326 (view as bug list)
Depends on: 189484
Blocks: 176199 188402 189202
  Show dependency treegraph
 
Reported: 2018-08-15 23:00 PDT by Myles C. Maxfield
Modified: 2018-10-13 15:32 PDT (History)
3 users (show)

See Also:


Attachments
Patch (7.39 KB, patch)
2018-09-06 10:17 PDT, Thomas Denney
no flags Details | Formatted Diff | Diff
Patch (14.98 KB, patch)
2018-09-10 16:19 PDT, Thomas Denney
no flags Details | Formatted Diff | Diff
Patch (16.19 KB, patch)
2018-09-10 17:01 PDT, Thomas Denney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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