Bug 195925 - [WHLSL] Implement property resolver
Summary: [WHLSL] Implement property resolver
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
: 195788 (view as bug list)
Depends on:
Blocks: 197448
  Show dependency treegraph
 
Reported: 2019-03-18 18:36 PDT by Myles C. Maxfield
Modified: 2019-05-23 15:41 PDT (History)
11 users (show)

See Also:


Attachments
WIP (28.48 KB, patch)
2019-03-18 18:38 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (40.01 KB, patch)
2019-03-19 00:17 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (40.61 KB, patch)
2019-03-19 17:58 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (42.41 KB, patch)
2019-03-19 19:35 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (114.00 KB, patch)
2019-03-20 18:02 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (117.51 KB, patch)
2019-03-21 16:58 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (120.24 KB, patch)
2019-03-21 17:18 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (130.67 KB, patch)
2019-03-21 19:21 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (144.61 KB, patch)
2019-03-22 17:25 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (153.28 KB, patch)
2019-05-14 23:15 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (156.44 KB, patch)
2019-05-21 12:00 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (157.97 KB, patch)
2019-05-21 13:48 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Simple dot expressions work (159.62 KB, patch)
2019-05-21 17:56 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Simple dot expressions work (159.65 KB, patch)
2019-05-21 18:34 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Simple dot expressions work (159.67 KB, patch)
2019-05-21 18:46 PDT, Myles C. Maxfield
sbarati: review+
Details | Formatted Diff | Diff
Patch for committing (222.81 KB, patch)
2019-05-23 00:41 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2019-03-18 18:36:42 PDT
[WHLSL] Implement property resolver
Comment 1 Myles C. Maxfield 2019-03-18 18:38:05 PDT
Created attachment 365100 [details]
WIP
Comment 2 Myles C. Maxfield 2019-03-18 18:39:29 PDT
Left-values can be created by dereference expressions, property accesses (dot expressions and index expressions), and variable references.
Comment 3 Myles C. Maxfield 2019-03-18 18:47:54 PDT
(a, b).c = d;
Comment 4 Myles C. Maxfield 2019-03-18 18:55:37 PDT
There are 4 cases:

1) Only an ander
2) Only a getter
3) Only a setter
4) A getter and a setter

In case 4, we need to make sure the types match.
Comment 5 Myles C. Maxfield 2019-03-18 19:29:49 PDT
Turns out the spec says that comma expressions are always rvalues.
Comment 6 Myles C. Maxfield 2019-03-18 22:59:35 PDT
782 For every declaration of a function with a name of the form ``operator&.field`` for some name ``field`` with argument type ``thread T1*`` and return type ``thread T2*``:
 783
 784     #. Add a declaration of a function ``operator.field=`` for the same name ``field``, with argument types ``T1`` and ``T2``, and return type ``T1``
 785     #. Add a declaration of a function ``operator.field`` for the same name ``field``, with argument type ``T1`` and return type ``T2``
 786
 787 For every declaration of the function ``operator&[]`` with argument types ``thread T1*`` and ``uint32``, and return type ``thread T2*``:
 788
 789     #. Add a declaration of a function ``operator[]=`` with argument types ``T1``, ``uint32`` and ``T2``, and return type ``T1``
 790     #. Add a declaration of a function ``operator[]`` with argument types ``T1`` and ``uint32``, and return type ``T2``
 791
 792 For every function with a name of the form ``operator.field=`` for some name ``field`` which is defined:
 793
 794     #. There must be a function with the name ``operator.field`` (for the same name ``field``) which is defined
 795     #. For each declaration of the former with arguments type ``(t1, t2)``, there must be a declaration of the latter with argument type ``(t1)``, and return type ``t2``
 796
 797 If a function with the name ``operator[]=`` is defined:
 798
 799     #. There must be a function with the name ``operator[]`` which is defined
 800     #. For each declaration of the former with arguments type ``(t1, t2, t3)``, there must be a declaration of the latter with argument type ``(t1, t2)``, and return type ``t3``
 801
 802 If there are two function declarations with the same names, number of parameters, types of their parameters, then the program is invalid.
Comment 7 Myles C. Maxfield 2019-03-18 23:57:55 PDT
We should make sure things like 5.foo work.
Comment 8 Myles C. Maxfield 2019-03-19 00:17:35 PDT
Created attachment 365130 [details]
WIP
Comment 9 Myles C. Maxfield 2019-03-19 00:23:42 PDT
We should also make sure we disallow set-only fields and fields with multiple setters.
Comment 10 Myles C. Maxfield 2019-03-19 16:02:17 PDT
<rdar://problem/48219643>
Comment 11 Myles C. Maxfield 2019-03-19 17:58:29 PDT
Created attachment 365275 [details]
WIP
Comment 12 Build Bot 2019-03-19 18:02:01 PDT
Attachment 365275 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Myles C. Maxfield 2019-03-19 19:35:08 PDT
Created attachment 365291 [details]
WIP
Comment 14 Myles C. Maxfield 2019-03-20 18:02:30 PDT
Created attachment 365455 [details]
WIP
Comment 15 Myles C. Maxfield 2019-03-21 16:58:43 PDT
Created attachment 365648 [details]
WIP
Comment 16 Myles C. Maxfield 2019-03-21 17:18:26 PDT
Created attachment 365651 [details]
WIP
Comment 17 Myles C. Maxfield 2019-03-21 19:21:40 PDT
Created attachment 365669 [details]
WIP
Comment 18 Myles C. Maxfield 2019-03-22 17:25:30 PDT
Created attachment 365787 [details]
WIP
Comment 19 Myles C. Maxfield 2019-04-17 15:25:08 PDT
*** Bug 195788 has been marked as a duplicate of this bug. ***
Comment 20 Justin Fan 2019-05-01 14:43:14 PDT
Example shader that should work after this patch:

vertex float4 vertex_main(uint id : SV_VertexID) : SV_Position
{
    float4 out;

    switch (id) {
    case 0:
        out = float4(-1, 1, 0, 1);
        break;
    case 1:
        out = float4(-1, -1, 0, 1);
        break;
    case 2:
        out = float4(1, 1, 0, 1);
        break;
    default:
        out = float4(1, -1, 0, 1);
        break;
    }
    
    return out;
}

fragment float4 fragment_main() : SV_Target 0 
{
    return float4(0, 1, 0, 1);
}
Comment 21 Justin Fan 2019-05-01 14:43:49 PDT
^for simple-triangle-strip.html in our layout tests.
Comment 22 Justin Fan 2019-05-01 15:03:00 PDT
Comment on attachment 365787 [details]
WIP

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

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLPropertyAccessExpression.h:62
> +    FunctionDeclaration* getFunction() { return m_getFunction; }

#include "WHLSLFunctionDeclaration.h"

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:113
>  }

In FunctionDefinitionWriter::visit(AST::FunctionDefinition&) [lines 217, 234]:

Ignore all 'ASSERT(m_stack.isEmpty());'

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:233
>          checkErrorAndVisit(functionDefinition.block());

In FunctionDefintionWriter::visit(AST::AssignmentExpression&) [line 489]: 

ADD:

    if (is<AST::DereferenceExpression>(assignmentExpression.left())) {
        checkErrorAndVisit(downcast<AST::DereferenceExpression>(assignmentExpression.left()).pointer());
        auto leftName = m_stack.takeLast();
        checkErrorAndVisit(assignmentExpression.right());
        auto rightName = m_stack.takeLast();
        m_stringBuilder.append(makeString('*', leftName, " = ", rightName, ";\n"));
        m_stack.append(rightName);
        return;
    }

[previously line 503]:

REPLACE:

m_stack.append(leftName); -> m_stack.append(rightName);

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:30
> +

#include "WHLSLAssignmentExpression.h"
#include "WHLSLCallExpression.h"
#include "WHLSLCommaExpression.h"
#include "WHLSLDereferenceExpression.h"
#include "WHLSLDotExpression.h"
#include "WHLSLFunctionDeclaration.h"
#include "WHLSLFunctionDefinition.h"
#include "WHLSLMakePointerExpression.h"
#include "WHLSLPointerType.h"
#include "WHLSLReadModifyWriteExpression.h"
#include "WHLSLVariableDeclaration.h"
#include "WHLSLVariableReference.h"

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:182
> +        ASSERT(iterator->base().typeAnnotation());

Move 'chain.append(*iterator);' here to prevent underflow math later based on chain.size().
Comment 23 Justin Fan 2019-05-01 17:33:26 PDT
Also, add the float4() constructor to WHLSLStandardLibrary.txt:

float4 float4(float x, float y, float z, float w) {
    float4 result;
    result.x = x;
    result.y = y;
    result.z = z;
    result.w = w;
    return result;
}
Comment 24 Myles C. Maxfield 2019-05-14 23:15:35 PDT
Created attachment 369928 [details]
WIP
Comment 25 Myles C. Maxfield 2019-05-21 09:44:12 PDT
(In reply to Justin Fan from comment #23)
> Also, add the float4() constructor to WHLSLStandardLibrary.txt:
> 
> float4 float4(float x, float y, float z, float w) {
operator float4(float x, float y, float z, float w) {
>     float4 result;
>     result.x = x;
>     result.y = y;
>     result.z = z;
>     result.w = w;
>     return result;
> }
Comment 26 Myles C. Maxfield 2019-05-21 12:00:30 PDT
Created attachment 370332 [details]
WIP
Comment 27 Myles C. Maxfield 2019-05-21 13:48:32 PDT
Created attachment 370338 [details]
WIP
Comment 28 Myles C. Maxfield 2019-05-21 17:56:27 PDT
Created attachment 370361 [details]
Simple dot expressions work
Comment 29 Myles C. Maxfield 2019-05-21 18:34:41 PDT
Created attachment 370366 [details]
Simple dot expressions work
Comment 30 Myles C. Maxfield 2019-05-21 18:46:25 PDT
Created attachment 370369 [details]
Simple dot expressions work
Comment 31 Robin Morisset 2019-05-22 14:56:17 PDT
Comment on attachment 370369 [details]
Simple dot expressions work

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

I've not checked every single line closely, but it looks reasonable overall. I've just a few nits that I've added inline.

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLExpression.h:100
> +    Optional<TypeAnnotation> m_typeAnnotation;

I am not sure I understand why it is optional when {RightValue/Abstract left-value/Left value} should cover all types.
Is it because the types is built over time and is not available from the start?

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLPropertyAccessExpression.h:110
> +    FunctionDeclaration* m_andFunction { nullptr };

What is the difference between m_andFunction and m_threadAndFunction?

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLVariableDeclaration.h:82
> +using VariableDeclarations = Vector<UniqueRef<VariableDeclaration>>;

Can you mention this change in the changelog and explain its reasoning?
Considering how much of the patch it is, it would probably have been best as a separate patch.

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

As long as this path is only for *native* function declarations, I think it is not a risk.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:623
> +static Optional<UniqueRef<AST::UnnamedType>> commit(ResolvingType& resolvingType)

Could you either give a more informative name to this function or some comment?
It is far from obvious what it does.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:792
> +    auto addressSpaceAddResult = m_typeAnnotations.add(&expression, WTFMove(typeAnnotation));

Nit: maybe rename it typeAnnotationAddResult ?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:985
> +        andFunction = resolveFunctionOverloadImpl(dotExpression.possibleAndOverloads(), andArgumentTypes, nullptr);

Nit: It looks like { baseInfo->resolvingType } instead of andArgumentTypes might be a bit more readable and let you remove a couple lines.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:1028
> +    if (setFunction && andFunction) {

Does this mean that we only detect this case when for types that we use in a dot expression?
This would be a deviation from the spec that would reject any program with an ander and a setter that collide, whether or not they are dead code.

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

I agree, it does not seem right at all. I don't understand the way the checker and in particular these variable references work well enough to suggest a fix though.

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

This looks like it should only execute in the case where we are doing a RMW operation?
Or at least we could skip a lot of that in the normal case.

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

I think it is right.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:744
> +    // FIXME: What about function call arguments?

A function call is always a right-value.
Comment 32 Saam Barati 2019-05-22 15:49:25 PDT
Comment on attachment 370369 [details]
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)...);
}
Comment 33 Myles C. Maxfield 2019-05-23 00:30:23 PDT
Comment on attachment 370369 [details]
Simple dot expressions work

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

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

When writing the compiler, I chose to use full names instead of abbreviations because they make things clearer. E.g. "TypeDefinition" instead of "typedef" and "StructureDefinition" instead of "struct". The logic is just that if I can make it easier to read and understand the code, it's worth the extra bytes in the source files on our hard drives. Characters are cheap and aren't an endangered species; we (as humans) are not running out of characters.

>> 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); }

Cool.

>> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLExpression.h:100
>> +    Optional<TypeAnnotation> m_typeAnnotation;
> 
> I am not sure I understand why it is optional when {RightValue/Abstract left-value/Left value} should cover all types.
> Is it because the types is built over time and is not available from the start?

Yep! It's not specified in the constructor.

However, we should probably treat the getter above with the Saam treatment and ASSERT() that m_typeAnnotation is non-nullopt and make the return type of the getter just TypeAnnotation.

>> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLPropertyAccessExpression.h:110
>> +    FunctionDeclaration* m_andFunction { nullptr };
> 
> What is the difference between m_andFunction and m_threadAndFunction?

The and function is used in this case:

struct Foo { ... }

device int* operator&.bar(device Foo*) { ... }

compute void computeShader(device Foo[] foos) {
    int baz = foos[0].bar;
}

The thread ander is used in this case:

struct Foo { ... }

thread int* operator&.bar(thread Foo*) { ... }

struct Baz { ... }

Foo operator.quux(Baz) { ... }

compute void computeShader() {
    Baz b = ...;
    int i = b.quux.bar;
}

We need to call the "bar" ander on the result of "b.quux", however, b.quux is an rvalue and thus doesn't have an address, so we can't just call the "bar" ander on it. Therefore, we have to save the result of "b.quux" in a local thread variable and call the thread ander with the address of the local variable.

>> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLVariableDeclaration.h:82
>> +using VariableDeclarations = Vector<UniqueRef<VariableDeclaration>>;
> 
> Can you mention this change in the changelog and explain its reasoning?
> Considering how much of the patch it is, it would probably have been best as a separate patch.

You're right.

>>> 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.
>> 
>> As long as this path is only for *native* function declarations, I think it is not a risk.
> 
> Can you link to a bug here so we don't forget?

Consider the following:

struct Foo {
    int length;
}

synthesizeStructureAccessors() will create a getter for that field (just like it would create a getter for any other field) and this new getter will be named "operator.length". It will be a NativeFunctionDeclaration because it's being generated by synthesizeStructureAccessors(). Then, this code will recognize the name, and erroneously treat it as if it was .length on an array reference.

https://bugs.webkit.org/show_bug.cgi?id=198077

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:623
>> +static Optional<UniqueRef<AST::UnnamedType>> commit(ResolvingType& resolvingType)
> 
> Could you either give a more informative name to this function or some comment?
> It is far from obvious what it does.

The functions directly above it are called "matchAndCommit()", and WHLSLInferTypes has a bunch of matchAndCommit() and a commit(). The name "commit" was taken from the JS compiler, where it means "assigning a concrete type to an abstract type (like a literal)".

What should we name it instead? Or should we document the meaning a different way (comments?)

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:1028
>> +    if (setFunction && andFunction) {
> 
> Does this mean that we only detect this case when for types that we use in a dot expression?
> This would be a deviation from the spec that would reject any program with an ander and a setter that collide, whether or not they are dead code.

https://bugs.webkit.org/show_bug.cgi?id=198143

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

You're right!

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

cooooooool

>>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:404
>>> +    auto modifyResult = modify(downcast<AST::DotExpression>(assignmentExpression.left()), [&](Optional<UniqueRef<AST::Expression>>&&) -> Optional<ModificationResult> {
>> 
>> This looks like it should only execute in the case where we are doing a RMW operation?
>> Or at least we could skip a lot of that in the normal case.
> 
> taking rvalue ref and using it as argument is kinda weird.

Robin: I don't think I understand your comment. Do you think you could clarify it?

Saam: This function is supposed to take ownership of its argument. Should I just change it to [&](Optional<UniqueRef<AST::Expression>>)?

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:635
>> +            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)...);
> }

This is a good idea, but I'd like to do it in a follow-up patch.

https://bugs.webkit.org/show_bug.cgi?id=198175
Comment 34 Myles C. Maxfield 2019-05-23 00:41:05 PDT
Created attachment 370493 [details]
Patch for committing
Comment 35 WebKit Commit Bot 2019-05-23 02:29:35 PDT
Comment on attachment 370493 [details]
Patch for committing

Clearing flags on attachment: 370493

Committed r245680: <https://trac.webkit.org/changeset/245680>
Comment 36 Saam Barati 2019-05-23 09:08:57 PDT
Comment on attachment 370369 [details]
Simple dot expressions work

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

>>> 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.
> 
> When writing the compiler, I chose to use full names instead of abbreviations because they make things clearer. E.g. "TypeDefinition" instead of "typedef" and "StructureDefinition" instead of "struct". The logic is just that if I can make it easier to read and understand the code, it's worth the extra bytes in the source files on our hard drives. Characters are cheap and aren't an endangered species; we (as humans) are not running out of characters.

My argument was not in favor of doing this to save typing, rather, that “lvalue” and “rvalue” are easier to understand.
Comment 37 Myles C. Maxfield 2019-05-23 15:41:51 PDT
Committed r245722: <https://trac.webkit.org/changeset/245722>