Bug 201251 – [WHLSL] Add back a version of the property resolver

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

[Saam Barati]

The goal will be to:
- make each property access's base effect free so metal codegen can emit 
- make the leftmost base of RMW effect free so we can emit the "leftValue()" twice

[Saam Barati]

and add back all the property resolver tests that make sense in this new world

[Saam Barati]

also we'll want the "high zombie finder" to have some new rules

[Saam Barati]

Created attachment 378121 [details]
WIP

it begins

[Saam Barati]

Created attachment 378142 [details]
WIP

it seems to work.

[Saam Barati]

Created attachment 378151 [details]
WIP

I think it's done, I just want to write some more tests

[Saam Barati]

Created attachment 378251 [details]
patch

[Robin Morisset]

Comment on attachment 378251 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378251&action=review

r=me

> Source/WebCore/ChangeLog:9
> +        The goal of the new property resolver phase is two allow two things:

two allow => to allow

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:90
> +        while (true) {

This pattern of
```
while(true) {
..
chain.append(current);
if (is<AST::PropertyAccessExpression>(current.base()))
  continue;
Tons of code that only executes once
break;
}
```
is not super readable. It is probably not worth modifying now, but I am curious, why not just use the loop to accumulate the chain:
```
while (is<AST::PropertyAccessExpression>(current.base())) {
  append to chain
  current = *currentPtr;
}
tons of code that manipulate current but are now outside of the loop
```

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:245
> +        RELEASE_ASSERT(m_variables.isEmpty());

There is no guarantee that the WTFMove later on will empty-out m_variables.
I think you need to either empty it by hand, or more simply to construct a different PropertyResolver for each function definition in resolveProperties.

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLReadModifyWriteExpression.h:63
> +    ReadModifyWriteExpression(CodeLocation location, UniqueRef<Expression> leftValue, UniqueRef<VariableDeclaration> oldValue, UniqueRef<VariableDeclaration> newValue)

I am unclear on whether it is better to pass UniqueRef<Expression> or UniqueRef<Expression>&& here.

[Saam Barati]

Comment on attachment 378251 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378251&action=review

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:90
>> +        while (true) {
> 
> This pattern of
> ```
> while(true) {
> ..
> chain.append(current);
> if (is<AST::PropertyAccessExpression>(current.base()))
>   continue;
> Tons of code that only executes once
> break;
> }
> ```
> is not super readable. It is probably not worth modifying now, but I am curious, why not just use the loop to accumulate the chain:
> ```
> while (is<AST::PropertyAccessExpression>(current.base())) {
>   append to chain
>   current = *currentPtr;
> }
> tons of code that manipulate current but are now outside of the loop
> ```

fixed.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:245
>> +        RELEASE_ASSERT(m_variables.isEmpty());
> 
> There is no guarantee that the WTFMove later on will empty-out m_variables.
> I think you need to either empty it by hand, or more simply to construct a different PropertyResolver for each function definition in resolveProperties.

that's how move constructor of Vector works. It swaps with empty vector, so it should be emptied out.

>> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLReadModifyWriteExpression.h:63
>> +    ReadModifyWriteExpression(CodeLocation location, UniqueRef<Expression> leftValue, UniqueRef<VariableDeclaration> oldValue, UniqueRef<VariableDeclaration> newValue)
> 
> I am unclear on whether it is better to pass UniqueRef<Expression> or UniqueRef<Expression>&& here.

I prefer what I did here since it expressed the fact that this function now has ownership over the UniqueRef. I'm not a fan of passing around rvalue refs since it doesn't actually guarantee that ownership will be transferred.

[Saam Barati]

thanks for the review.


Landed in:
https://trac.webkit.org/changeset/249630/webkit

[Saam Barati]

landed watchOS build fix in:
https://trac.webkit.org/changeset/249664/webkit

[Radar WebKit Bug Importer]

<rdar://problem/55196672>

[Robin Morisset]

This patch turns out to create comma expressions on the left-side of assignments.
It is not something which could otherwise happen. Do we want to support it, or to change this pass?