We transform WHLSL expressions like "a < b * c" into something like: auto temporary1 = b * c; auto result = a < temporary1; This works in the general case, but not if we have something like: while (a < b * c) { ... } MSL doesn't support lambdas, so we can't just throw the statements into a lambda. There are two solutions to this problem: 1) Turn all loops into infinite loops, and emulate their expressions by emitting code in all the necessary places. So, something like: while (true) { auto temporary1 = b * c; auto result = a < temporary1; if (!result) break; } 2) Generate a functor class, passing in references to all referenced variables. So, something like: class Functor { Functor(int& a, int& b, int& c) : a(a), b(b), c(c) {} bool operator()() { auto temporary1 = b * c; auto result = a < temporary1; return result; } private: int& a; int& b; int& c; }; while (Functor(a, b, c)()) { ... }
<rdar://problem/50746309>
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.