Bug 198399 - [WHLSL] Educate the property resolver about IndexExpressions
Summary: [WHLSL] Educate the property resolver about IndexExpressions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on: 197448 198198
Blocks: 198163
  Show dependency treegraph
 
Reported: 2019-05-30 17:23 PDT by Myles C. Maxfield
Modified: 2019-06-12 20:44 PDT (History)
9 users (show)

See Also:


Attachments
WIP (49.61 KB, patch)
2019-05-30 17:23 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (49.63 KB, patch)
2019-05-30 17:27 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (157.68 KB, patch)
2019-05-31 00:00 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (104.88 KB, patch)
2019-05-31 18:53 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
indexer-ander-abstract-lvalue crashes (124.01 KB, patch)
2019-05-31 23:59 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (130.99 KB, patch)
2019-06-01 23:38 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (165.12 KB, patch)
2019-06-02 00:54 PDT, Myles C. Maxfield
sbarati: review+
Details | Formatted Diff | Diff
Try again (164.69 KB, patch)
2019-06-11 11:12 PDT, Myles C. Maxfield
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-highsierra (3.01 MB, application/zip)
2019-06-11 13:18 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2019-05-30 17:23:06 PDT
[WHLSL] Educate the property resolver about IndexExpressions
Comment 1 Myles C. Maxfield 2019-05-30 17:23:54 PDT
Created attachment 371002 [details]
WIP
Comment 2 Myles C. Maxfield 2019-05-30 17:27:00 PDT
Created attachment 371003 [details]
WIP
Comment 3 Myles C. Maxfield 2019-05-30 17:29:13 PDT
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.
Comment 4 Myles C. Maxfield 2019-05-31 00:00:49 PDT
Created attachment 371042 [details]
WIP
Comment 5 Myles C. Maxfield 2019-05-31 18:53:27 PDT
Created attachment 371101 [details]
WIP
Comment 6 Myles C. Maxfield 2019-05-31 23:59:41 PDT
Created attachment 371110 [details]
indexer-ander-abstract-lvalue crashes
Comment 7 Myles C. Maxfield 2019-06-01 23:38:13 PDT
Created attachment 371138 [details]
WIP
Comment 8 Myles C. Maxfield 2019-06-02 00:54:17 PDT
Created attachment 371139 [details]
Patch
Comment 9 Saam Barati 2019-06-03 09:50:26 PDT
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>>&"?
Comment 10 Myles C. Maxfield 2019-06-05 12:18:39 PDT
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.
Comment 11 Myles C. Maxfield 2019-06-05 19:17:24 PDT
Committed r246138: <https://trac.webkit.org/changeset/246138>
Comment 12 Radar WebKit Bug Importer 2019-06-05 19:18:21 PDT
<rdar://problem/51466333>
Comment 13 Truitt Savell 2019-06-07 17:04:02 PDT
Reverted r246138 for reason:

Broke internal builds

Committed r246225: <https://trac.webkit.org/changeset/246225>
Comment 14 Myles C. Maxfield 2019-06-11 11:12:17 PDT
Created attachment 371857 [details]
Try again
Comment 15 Build Bot 2019-06-11 13:18:19 PDT
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
Comment 16 Build Bot 2019-06-11 13:18:21 PDT
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
Comment 17 Myles C. Maxfield 2019-06-12 15:59:45 PDT
Committed r246385: <https://trac.webkit.org/changeset/246385>
Comment 18 Myles C. Maxfield 2019-06-12 20:44:30 PDT
Committed r246390: <https://trac.webkit.org/changeset/246390>