[WHLSL] Implement property resolver
Created attachment 365100 [details] WIP
Left-values can be created by dereference expressions, property accesses (dot expressions and index expressions), and variable references.
(a, b).c = d;
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.
Turns out the spec says that comma expressions are always rvalues.
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.
We should make sure things like 5.foo work.
Created attachment 365130 [details] WIP
We should also make sure we disallow set-only fields and fields with multiple setters.
<rdar://problem/48219643>
Created attachment 365275 [details] WIP
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.
Created attachment 365291 [details] WIP
Created attachment 365455 [details] WIP
Created attachment 365648 [details] WIP
Created attachment 365651 [details] WIP
Created attachment 365669 [details] WIP
Created attachment 365787 [details] WIP
*** Bug 195788 has been marked as a duplicate of this bug. ***
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); }
^for simple-triangle-strip.html in our layout tests.
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().
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; }
Created attachment 369928 [details] WIP
(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; > }
Created attachment 370332 [details] WIP
Created attachment 370338 [details] WIP
Created attachment 370361 [details] Simple dot expressions work
Created attachment 370366 [details] Simple dot expressions work
Created attachment 370369 [details] Simple dot expressions work
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 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 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
Created attachment 370493 [details] Patch for committing
Comment on attachment 370493 [details] Patch for committing Clearing flags on attachment: 370493 Committed r245680: <https://trac.webkit.org/changeset/245680>
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.
Committed r245722: <https://trac.webkit.org/changeset/245722>