Bug 193434 – [WHLSL] Implement the loop checker

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

[Myles C. Maxfield]

[WHLSL] Implement the loop checker

[Myles C. Maxfield]

Created attachment 359135 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 359135 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLoopChecker.h:32
> +#include "WHLSLFunctionAttribute.h"
> +#include "WHLSLSemantic.h"
> +#include "WHLSLTypeArgument.h"

These can be deleted.

[Myles C. Maxfield]

Comment on attachment 359135 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        This is a translation of https://github.com/gpuweb/WHLSL/blob/master/Source/LoopChecker.mjs into C++.

Need a "reviewed by" line

[Robin Morisset]

Comment on attachment 359135 [details]
Patch

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

r=me
I understand that you are translating the reference implementation file by file for simplicity, but I am a bit wary of the distance between both implementations on the one hand and the spec on the other. I guess I will try to update at least the reference implementation later on to follow the structure of the spec (in particular doing all of the type checking and validation in one pass, using the idea of "behaviors" aka types for statements that I used in the spec).

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLoopChecker.cpp:53
> +        if (m_loopDepth || m_switchDepth) {

Not sure this part is necessary: I don't think we allow function definitions within the bodies of other functions anyway.

[Saam Barati]

Comment on attachment 359135 [details]
Patch

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

LGTM

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLoopChecker.cpp:44
> +class LoopChecker : public Visitor {

I feel like a comment explaining what this does would be helpful. "findHighZombies" is not the most intuitive name.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLoopChecker.cpp:53
>> +        if (m_loopDepth || m_switchDepth) {
> 
> Not sure this part is necessary: I don't think we allow function definitions within the bodies of other functions anyway.

Perhaps an assert?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLoopChecker.cpp:54
> +            setError();

I haven't reviewed other changes, so this is just a general comment, but don't we want error messages here?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLoopChecker.cpp:64
> +        ++m_loopDepth;

style: Maybe use:
SetForScope scope(m_loopDepth, m_loopDepth + 1) ?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLoopChecker.cpp:72
> +        ++m_loopDepth;

ditto

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLoopChecker.cpp:82
> +        WTF::visit(WTF::makeVisitor([&](AST::VariableDeclarationsStatement& variableDeclarationsStatement) {

style: maybe auto&?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLoopChecker.cpp:84
> +        }, [&](UniqueRef<AST::Expression>& expression) {

style: maybe auto&?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLoopChecker.cpp:92
> +        ++m_loopDepth;

ditto about SetForScope

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLoopChecker.cpp:102
> +        ++m_switchDepth;

ditto SetForScope

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLoopChecker.cpp:149
> +}
> +
> +}

Should we add the `// namespace X` comments here?

[Myles C. Maxfield]

Comment on attachment 359135 [details]
Patch

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

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLoopChecker.cpp:54
>> +            setError();
> 
> I haven't reviewed other changes, so this is just a general comment, but don't we want error messages here?

Yep.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLoopChecker.cpp:82
>> +        WTF::visit(WTF::makeVisitor([&](AST::VariableDeclarationsStatement& variableDeclarationsStatement) {
> 
> style: maybe auto&?

Visitor needs to know the parameter types.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLoopChecker.cpp:149
>> +}
> 
> Should we add the `// namespace X` comments here?

I'll do this in a follow-up patch.

[Myles C. Maxfield]

Committed r240018: <https://trac.webkit.org/changeset/240018>

[Radar WebKit Bug Importer]

<rdar://problem/47301321>

[Myles C. Maxfield]

Comment on attachment 359135 [details]
Patch

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

>>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLoopChecker.cpp:149
>>> +}
>> 
>> Should we add the `// namespace X` comments here?
> 
> I'll do this in a follow-up patch.

https://bugs.webkit.org/show_bug.cgi?id=193471

[Saam Barati]

Comment on attachment 359135 [details]
Patch

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

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLLoopChecker.cpp:64
>> +        ++m_loopDepth;
> 
> style: Maybe use:
> SetForScope scope(m_loopDepth, m_loopDepth + 1) ?

This may have been bad advice. I don't think this matters ATM, but if we encounter an AST node that ++m_XYZ, but doesn't --m_XYZ, then my suggestion of using SetForScope is not equivalent.

[Myles C. Maxfield]

Committed r240026: <https://trac.webkit.org/changeset/240026>