Bug 198399 – [WHLSL] Educate the property resolver about IndexExpressions

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

[Myles C. Maxfield]

[WHLSL] Educate the property resolver about IndexExpressions

[Myles C. Maxfield]

Created attachment 371002 [details]
WIP

[Myles C. Maxfield]

Created attachment 371003 [details]
WIP

[Myles C. Maxfield]

Comment on attachment 371003 [details]
WIP

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

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:782
> +    if (additionalArgument)

Instead of an additional argument, we can just downcast and call takeIndex() here instead.

[Myles C. Maxfield]

Created attachment 371042 [details]
WIP

[Myles C. Maxfield]

Created attachment 371101 [details]
WIP

[Myles C. Maxfield]

Created attachment 371110 [details]
indexer-ander-abstract-lvalue crashes

[Myles C. Maxfield]

Created attachment 371138 [details]
WIP

[Myles C. Maxfield]

Created attachment 371139 [details]
Patch

[Saam Barati]

Comment on attachment 371139 [details]
Patch

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

r=me. Awesome! Just a couple nits below.

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLReferenceType.h:56
> +    bool isReferenceType() const override { return true; }

👍🏼

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:358
> +            argumentTypes.append((*functionDefinition.parameters()[i]->type())->clone());

👍🏼

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:98
> +            auto variableReference = makeUniqueRef<AST::VariableReference>(AST::VariableReference::wrap(*indexVariable));
> +            ASSERT(indexVariable->type());
> +            variableReference->setType(indexVariable->type()->clone());
> +            variableReference->setTypeAnnotation(AST::LeftValue { AST::AddressSpace::Thread }); // FIXME: https://bugs.webkit.org/show_bug.cgi?id=198169 Is this right?
> +            arguments.append(WTFMove(variableReference));

nit: You do this both along the "relevantAnder" path and the "!relevantAnder" path. Maybe you can move it above the "if"

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:154
> +            auto variableReference = makeUniqueRef<AST::VariableReference>(AST::VariableReference::wrap(*indexVariable));
> +            ASSERT(indexVariable->type());
> +            variableReference->setType(indexVariable->type()->clone());
> +            variableReference->setTypeAnnotation(AST::LeftValue { AST::AddressSpace::Thread }); // FIXME: https://bugs.webkit.org/show_bug.cgi?id=198169 Is this right?
> +            arguments.append(WTFMove(variableReference));

nit: ditto

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:247
> +        intermediateVariables.uncheckedAppend(makeUniqueRef<AST::VariableDeclaration>(Lexer::Token(propertyAccessExpression.origin()), AST::Qualifiers(), propertyAccessExpression.resolvedType().clone(), String(), WTF::nullopt, WTF::nullopt));

nit: You no longer need to wrap calls to origin() in copy constructor, since AST::Value::origin() returns Lexer::Token. This nit applies to a bunch of other places in this patch too.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:259
> +    // However, if we did this, we would have to run foo() twice, which would be incorrect.
> +    // Instead, we need to save foo() and b into more temporary variables.
> +    // These temporary variables are parallel to "chain" above, with nullopt referring to a DotExpression (which doesn't have an index value to save to a variable).

nit: maybe just for super-duper clarity, write out what the above will turn into w/ temps? (I know you do this in the changelog, but might also be nice to have here since you say above what *it should not* be.)

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:333
> +    auto appendIndexAssignment = [&](AST::PropertyAccessExpression& propertyAccessExpression, Optional<UniqueRef<AST::VariableDeclaration>>& indexVariable) {

nit: "const Optional<UniqueRef<AST::VariableDeclaration>>&" instead of "Optional<UniqueRef<AST::VariableDeclaration>>&"?

[Myles C. Maxfield]

Comment on attachment 371139 [details]
Patch

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

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:247
>> +        intermediateVariables.uncheckedAppend(makeUniqueRef<AST::VariableDeclaration>(Lexer::Token(propertyAccessExpression.origin()), AST::Qualifiers(), propertyAccessExpression.resolvedType().clone(), String(), WTF::nullopt, WTF::nullopt));
> 
> nit: You no longer need to wrap calls to origin() in copy constructor, since AST::Value::origin() returns Lexer::Token. This nit applies to a bunch of other places in this patch too.

Good idea, but doing this consistently would pollute this patch. I'll do it in a follow-up.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:333
>> +    auto appendIndexAssignment = [&](AST::PropertyAccessExpression& propertyAccessExpression, Optional<UniqueRef<AST::VariableDeclaration>>& indexVariable) {
> 
> nit: "const Optional<UniqueRef<AST::VariableDeclaration>>&" instead of "Optional<UniqueRef<AST::VariableDeclaration>>&"?

We take the index expression from it, so it gets mutated.

[Myles C. Maxfield]

Committed r246138: <https://trac.webkit.org/changeset/246138>

[Radar WebKit Bug Importer]

<rdar://problem/51466333>

[Truitt Savell]

Reverted r246138 for reason:

Broke internal builds

Committed r246225: <https://trac.webkit.org/changeset/246225>

[Myles C. Maxfield]

Created attachment 371857 [details]
Try again

[EWS Watchlist]

Comment on attachment 371857 [details]
Try again

Attachment 371857 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12445966

New failing tests:
compositing/repaint/scroller-with-foreground-layer-repaints.html

[EWS Watchlist]

Created attachment 371872 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Myles C. Maxfield]

Committed r246385: <https://trac.webkit.org/changeset/246385>

[Myles C. Maxfield]

Committed r246390: <https://trac.webkit.org/changeset/246390>