Bug 195808

Summary: [WHLSL] Implement loop expressions
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: WebGPUAssignee: 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 Flags
patch
mmaxfield: review+
patch for landing none

Description Myles C. Maxfield 2019-03-15 11:45:31 PDT
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)()) {
   ...
}
Comment 1 Radar WebKit Bug Importer 2019-05-13 17:08:08 PDT
<rdar://problem/50746309>
Comment 2 Saam Barati 2019-05-31 19:39:19 PDT
Created attachment 371103 [details]
patch
Comment 3 Myles C. Maxfield 2019-06-05 06:22:25 PDT
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 4 Myles C. Maxfield 2019-06-05 06:23:14 PDT
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 5 Saam Barati 2019-06-05 08:16:01 PDT
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 6 Myles C. Maxfield 2019-06-05 09:33:40 PDT
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 7 Saam Barati 2019-06-05 09:43:22 PDT
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 8 Saam Barati 2019-06-05 09:51:46 PDT
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.
Comment 9 Saam Barati 2019-06-05 10:12:15 PDT
Created attachment 371417 [details]
patch for landing
Comment 10 WebKit Commit Bot 2019-06-05 11:36:35 PDT
Comment on attachment 371417 [details]
patch for landing

Clearing flags on attachment: 371417

Committed r246121: <https://trac.webkit.org/changeset/246121>
Comment 11 WebKit Commit Bot 2019-06-05 11:36:37 PDT
All reviewed patches have been landed.  Closing bug.