Bug 198399

Created attachment 371002 [details] WIP

Created attachment 371003 [details] WIP

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.

Created attachment 371042 [details] WIP

Created attachment 371101 [details] WIP

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

Created attachment 371138 [details] WIP

Created attachment 371139 [details] Patch

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 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.

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

<rdar://problem/51466333>

Reverted r246138 for reason: Broke internal builds Committed r246225: <https://trac.webkit.org/changeset/246225>

Created attachment 371857 [details] Try again

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

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

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

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