Summary: | [WHLSL] Implement loop expressions | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||
Component: | WebGPU | Assignee: | Saam Barati <saam> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, jonlee, webkit-bug-importer | ||||||
Priority: | P1 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Myles C. Maxfield
2019-03-15 11:45:31 PDT
Created attachment 371103 [details]
patch
Comment on attachment 371103 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=371103&action=review > Source/WebCore/ChangeLog:46 > + while (0) { while(0)? The body of this loop will never execute, then. Are you sure you didn't mean do { } while (0);? This description could be a little more clear. It appears that by wrapping the body of the loop in a do/while loop, you're modelling "goto the bottom of the loop" as a "break". Therefore, WHLSL's "continue" is modeled as "breakOutOfCurrentLoop = false; break;" and WHLSL's "break" is modeled as "breakOutOfCurrentLoop = true; break;" Comment on attachment 371103 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=371103&action=review >> Source/WebCore/ChangeLog:46 >> + while (0) { > > while(0)? The body of this loop will never execute, then. Are you sure you didn't mean do { } while (0);? > > This description could be a little more clear. It appears that by wrapping the body of the loop in a do/while loop, you're modelling "goto the bottom of the loop" as a "break". Therefore, WHLSL's "continue" is modeled as "breakOutOfCurrentLoop = false; break;" and WHLSL's "break" is modeled as "breakOutOfCurrentLoop = true; break;" Also, a for loop example would be more interesting than a while loop example here. Comment on attachment 371103 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=371103&action=review >>> Source/WebCore/ChangeLog:46 >>> + while (0) { >> >> while(0)? The body of this loop will never execute, then. Are you sure you didn't mean do { } while (0);? >> >> This description could be a little more clear. It appears that by wrapping the body of the loop in a do/while loop, you're modelling "goto the bottom of the loop" as a "break". Therefore, WHLSL's "continue" is modeled as "breakOutOfCurrentLoop = false; break;" and WHLSL's "break" is modeled as "breakOutOfCurrentLoop = true; break;" > > Also, a for loop example would be more interesting than a while loop example here. Yeah I meant “ do {} while(0) “ Comment on attachment 371103 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=371103&action=review > Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:231 > + m_stringBuilder.append(makeString(m_breakOutOfCurrentLoopEarlyVariable, " = true;\n")); Can we ASSERT() that the variable is non-empty? > Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:237 > + m_stringBuilder.append("break;\n"); Ditto > Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:257 > + m_stringBuilder.append("while (1) {\n"); Nit: I think while (true) is clearer. > Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:266 > + m_stringBuilder.append("} while(0); \n"); Nit: while(false) > Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:267 > + m_stringBuilder.append(makeString("if (", m_breakOutOfCurrentLoopEarlyVariable, ") break;\n")); Cool. > Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:276 > + m_stack.takeLast(); Maybe add a short comment about why we need to takeLast() > Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:279 > + m_stringBuilder.append("} \n"); // while (1) { Spurious comment? > Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:284 > + emitLoop(false, &doWhileLoop.conditional(), nullptr, doWhileLoop.body()); I think WebKit style usually prefers not to have magic "true" or "false" values, instead preferring to use an enum class with 2 values. > Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:297 > checkErrorAndVisit(expression); I know you didn't write this code, but I think we should scope these statements to just the loop, by surrounding the entire for loop with { }. It doesn't affect correctness, but it can at least tell us early if we've messed something up. Comment on attachment 371103 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=371103&action=review >> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:279 >> + m_stringBuilder.append("} \n"); // while (1) { > > Spurious comment? I meant to indicate this is where the "while 1" from above is closed. I can remove it since it may be more confusing than not having it. >> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:297 >> checkErrorAndVisit(expression); > > I know you didn't write this code, but I think we should scope these statements to just the loop, by surrounding the entire for loop with { }. It doesn't affect correctness, but it can at least tell us early if we've messed something up. 👍🏼 Comment on attachment 371103 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=371103&action=review >> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:276 >> + m_stack.takeLast(); > > Maybe add a short comment about why we need to takeLast() Will do. I guess a more elegant solution in the future would be to just make increment an effectful statement node. Created attachment 371417 [details]
patch for landing
Comment on attachment 371417 [details] patch for landing Clearing flags on attachment: 371417 Committed r246121: <https://trac.webkit.org/changeset/246121> All reviewed patches have been landed. Closing bug. |