Bug 176294 – Add "if" statements to WSL

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Add "if" statements to WSL

[Myles C. Maxfield]

Created attachment 319763 [details]
WIP

[Myles C. Maxfield]

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?

[Filip Pizlo]

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.

[Myles C. Maxfield]

Created attachment 320210 [details]
Patch

[Filip Pizlo]

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.

[Myles C. Maxfield]

Committed r221776: <http://trac.webkit.org/changeset/221776>

[Saam Barati]

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

[Saam Barati]

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

[Joseph Pecoraro]

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;

[Ryan Haddad]

(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

[Ryan Haddad]

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>

[Filip Pizlo]

(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.

[Ryan Haddad]

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

[Filip Pizlo]

Relanded in https://trac.webkit.org/changeset/221829/webkit

[Radar WebKit Bug Importer]

<rdar://problem/34693704>

[Myles C. Maxfield]

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