Bug 176294 - Add "if" statements to WSL
Summary: Add "if" statements to WSL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
: 176631 (view as bug list)
Depends on:
Blocks: 176199
  Show dependency treegraph
 
Reported: 2017-09-03 00:10 PDT by Myles C. Maxfield
Modified: 2018-10-13 17:04 PDT (History)
6 users (show)

See Also:


Attachments
WIP (10.64 KB, patch)
2017-09-03 00:11 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (21.11 KB, patch)
2017-09-07 17:13 PDT, Myles C. Maxfield
fpizlo: review+
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 2017-09-03 00:10:34 PDT
Add "if" statements to WSL
Comment 1 Myles C. Maxfield 2017-09-03 00:11:25 PDT
Created attachment 319763 [details]
WIP
Comment 2 Myles C. Maxfield 2017-09-03 10:26:52 PDT
Comment on attachment 319763 [details]
WIP

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

> Tools/WebGPUShadingLanguageRI/NameResolver.js:178
> +            let argumentIsType = node.typeArguments[i] instanceof Type || node.typeArguments[i] instanceof TypeOrVariableRef;

I'm not sure about this. We may need to have an intermediary pass where we replace all the TypeOrVariableRefs with the right thing. WDYT, Fil?
Comment 3 Filip Pizlo 2017-09-03 10:37:15 PDT
Comment on attachment 319763 [details]
WIP

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

>> Tools/WebGPUShadingLanguageRI/NameResolver.js:178
>> +            let argumentIsType = node.typeArguments[i] instanceof Type || node.typeArguments[i] instanceof TypeOrVariableRef;
> 
> I'm not sure about this. We may need to have an intermediary pass where we replace all the TypeOrVariableRefs with the right thing. WDYT, Fil?

It’s NameResolver’s job. Look for a typo in _resolveTypeArguments: typeArgument[i] should be typeArguments[i]. I fixed it in one of my patches.
Comment 4 Myles C. Maxfield 2017-09-07 17:13:03 PDT
Created attachment 320210 [details]
Patch
Comment 5 Filip Pizlo 2017-09-07 19:38:57 PDT
Comment on attachment 320210 [details]
Patch

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

R=me but I wouldn’t remove the type checks.

> Tools/WebGPUShadingLanguageRI/Checker.js:222
> +        // make sure that the operand is a bool.

I would still check though.
Comment 6 Myles C. Maxfield 2017-09-07 19:57:42 PDT
Committed r221776: <http://trac.webkit.org/changeset/221776>
Comment 7 Saam Barati 2017-09-08 00:09:32 PDT
Comment on attachment 320210 [details]
Patch

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

> Tools/WebGPUShadingLanguageRI/Checker.js:221
> +        // The parser forcably emits a bool cast on the operand, so we don't need to

Typo: should be forcibly

> Tools/WebGPUShadingLanguageRI/Parse.js:583
> +        if (token.text == "if")

Nit: it feels like it’s time to make this a switch instead of cascading “if”s
Comment 8 Saam Barati 2017-09-08 00:11:09 PDT
Comment on attachment 320210 [details]
Patch

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

>> Tools/WebGPUShadingLanguageRI/Checker.js:221
>> +        // The parser forcably emits a bool cast on the operand, so we don't need to
> 
> Typo: should be forcibly

Ignore me your landed patch doesn’t have this comment
Comment 9 Joseph Pecoraro 2017-09-08 17:46:24 PDT
Comment on attachment 320210 [details]
Patch

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

> Tools/WebGPUShadingLanguageRI/Evaluator.js:160
> +    visitIfStatement(node)
> +    {
> +        if (node.conditional.visit(this).loadValue())
> +            return node.body.visit(this);
> +        else if (node.elseBody)
> +            return node.elseBody.visit(this);
> +    }

Is the implicit return value expected? If so, maybe make it explicit, I don't see any other code expecting/depending on implicit return values.

    if (...)
        return node.body.visit(this);
    if (node.elseBody)
        return node.elseBody.visit(this);
    return undefined;
Comment 10 Ryan Haddad 2017-09-09 09:53:54 PDT
(In reply to Myles C. Maxfield from comment #6)
> Committed r221776: <http://trac.webkit.org/changeset/221776>

This change caused wsl-tests.yaml/Test.js.ftl-eager-no-cjit to time out on debug JSC bots. Details in https://bugs.webkit.org/show_bug.cgi?id=176631
Comment 11 Ryan Haddad 2017-09-09 09:56:59 PDT
Reverted r221776 for reason:

This change caused wsl-tests.yaml/Test.js to time out on Debug JSC bots.

Committed r221824: <http://trac.webkit.org/changeset/221824>
Comment 12 Filip Pizlo 2017-09-09 10:33:36 PDT
(In reply to Ryan Haddad from comment #11)
> Reverted r221776 for reason:
> 
> This change caused wsl-tests.yaml/Test.js to time out on Debug JSC bots.
> 
> Committed r221824: <http://trac.webkit.org/changeset/221824>

In the future, can we skip this test instead, if it times out?

Timeouts on this test are rarely real bugs, so rolling things out due to timeouts on this test doesn’t help us.
Comment 13 Ryan Haddad 2017-09-09 12:05:09 PDT
*** Bug 176631 has been marked as a duplicate of this bug. ***
Comment 14 Filip Pizlo 2017-09-09 15:08:14 PDT
Relanded in https://trac.webkit.org/changeset/221829/webkit
Comment 15 Radar WebKit Bug Importer 2017-09-27 12:39:35 PDT
<rdar://problem/34693704>
Comment 16 Myles C. Maxfield 2018-10-13 17:04:33 PDT
Migrated to https://github.com/gpuweb/WHLSL/issues/156