[webkit-reviews] review granted: [Bug 198399] [WHLSL] Educate the property resolver about IndexExpressions : [Attachment 371139] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 198399: [WHLSL] Educate the property resolver about IndexExpressions
https://bugs.webkit.org/show_bug.cgi?id=198399

Attachment 371139: Patch

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




--- Comment #9 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 371139
  --> https://bugs.webkit.org/attachment.cgi?id=371139
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(*indexVariab
le));
> +	       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(*indexVariab
le));
> +	       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>(L
exer::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>>&"?