Bug 195808 – [WHLSL] Implement loop expressions

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Myles C. Maxfield]

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)()) {
   ...
}

[Radar WebKit Bug Importer]

<rdar://problem/50746309>

[Saam Barati]

Created attachment 371103 [details]
patch

[Myles C. Maxfield]

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;"

[Myles C. Maxfield]

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.

[Saam Barati]

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) “

[Myles C. Maxfield]

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.

[Saam Barati]

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.

👍🏼

[Saam Barati]

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.

[Saam Barati]

Created attachment 371417 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 371417 [details]
patch for landing

Clearing flags on attachment: 371417

Committed r246121: <https://trac.webkit.org/changeset/246121>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.