Bug 176294

Summary: Add "if" statements to WSL
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, joepeck, keith_miller, ryanhaddad, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=176631
Bug Depends on:    
Bug Blocks: 176199    
Attachments:
Description Flags
WIP
none
Patch fpizlo: review+

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