[webkit-reviews] review granted: [Bug 195925] [WHLSL] Implement property resolver : [Attachment 370369] Simple dot expressions work

[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 195925: [WHLSL] Implement property resolver
https://bugs.webkit.org/show_bug.cgi?id=195925

Attachment 370369: Simple dot expressions work

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




--- Comment #32 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 370369
  --> https://bugs.webkit.org/attachment.cgi?id=370369
Simple dot expressions work

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

r=me

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLAddressSpace.h:46
> +struct LeftValue {

Name nit: Do we really want to go with "Left" instead of "L", "Right" instead
of "R"? I feel like most of the text I see on this just uses "L" and "R" as the
canonical name.

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLAssignmentExpression.h:58
> +    UniqueRef<Expression>&& takeRight() { return WTFMove(m_right); }

nit: This doesn't actually take the right, e.g, if someone just called:
takeRight();

The value wouldn't actually end up being moved, since it's just casted to.
Maybe this should return UniqueRef<Expression> instead?

So: UniqueRef<Expression> takeRight() { return WTFMove(m_right); }

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLPropertyAccessExpression.h:102
> +    UniqueRef<Expression>&& takeBase() { return WTFMove(m_base); }

ditto about "take"

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:145
> +    // FIXME: Authors can make a struct field named "length" too.
Autogenerated getters for those shouldn't take this codepath.

Can you link to a bug here so we don't forget?

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:169
> +	   auto mangledFieldName = [&](String& fieldName) -> String {

const String& as param?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:987
> +	       andReturnType =
&downcast<AST::PointerType>(andFunction->type()).elementType(); // FIXME:
Enforce the return of anders will always be a pointer

Can you file a bug and link it here?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:998
> +	       threadAndReturnType =
&downcast<AST::PointerType>(threadAndFunction->type()).elementType(); // FIXME:
Enforce the return of anders will always be a pointer

ditto

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:1036
> +    dotExpression.setGetFunction(getFunction);
> +    dotExpression.setAndFunction(andFunction);
> +    dotExpression.setThreadAndFunction(threadAndFunction);
> +    dotExpression.setSetFunction(setFunction);

name nit: why not getter/setter/ander instead of set/get/and?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:1060
> +    if (!variableReference.variable()->isAnonymous()) // FIXME: This doesn't
seem right.

let's have a bugzilla

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:190
> +    // Find the ".b" ".c" and ".d" expressions.

Nit: Since this code is kinda hairy, might be worth stating what the order of
this vector turns out to be too. I think it's "d", "c", "b"

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:199
> +	   ASSERT(is<AST::DotExpression>(iterator->base())); // FIXME: Make
this work with index expressions

bugzilla please

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:280
> +    for (size_t i = chain.size() - 1; i > 0; --i) {

nit: I think this is easier to read if you write this as
(site_t i = 0; i--;) and just use "i" below instead of "i - 1"

This is how we write backwards indexed loops in all of JSC. It looks funky at
first, but it becomes intuitive to pattern match and understand that it's
correct.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:292
> +	   variableReference->setTypeAnnotation(AST::LeftValue {
AST::AddressSpace::Thread }); // FIXME: Is this right?

bugzilla

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:304
> +    // ModificationResult(UniqueRef<AST::Expression>&&)

?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:404
> +    auto modifyResult =
modify(downcast<AST::DotExpression>(assignmentExpression.left()),
[&](Optional<UniqueRef<AST::Expression>>&&) -> Optional<ModificationResult> {

taking rvalue ref and using it as argument is kinda weird.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:635
> +	       static_assert(sizeof(AST::DereferenceExpression) <=
sizeof(AST::DotExpression), "Dot expressions need to be able to become
dereference expressions without updating backreferences");
> +	       void* location = &dotExpression;
> +	       dotExpression.~DotExpression();
> +	       auto* dereferenceExpression = new (location)
AST::DereferenceExpression(WTFMove(origin), WTFMove(callExpression));
> +	      
dereferenceExpression->setType(downcast<AST::PointerType>(andFunction->type()).
elementType().clone());
> +	       dereferenceExpression->setTypeAnnotation(AST::LeftValue {
downcast<AST::PointerType>(andFunction->type()).addressSpace() });

Can we make some kind of function that abstract this AST replacement for us?
Maybe like:

template <typename Old, typename New, typename ...Args>
New* replace(Old& old, Args&&... args)
{
    static_assert(sizeof(New) <= sizeof(Old));
    return new (&old) New(std::forward<Args>(args)...);
}