| Summary: | [WHLSL] Implement the loop checker | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||
| Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | dino, fpizlo, jonlee, justin_fan, rmorisset, saam, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Myles C. Maxfield
2019-01-14 21:37:30 PST
Created attachment 359135 [details]
Patch
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. 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 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. 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? 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. Committed r240018: <https://trac.webkit.org/changeset/240018> 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 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. Committed r240026: <https://trac.webkit.org/changeset/240026> |