The goal will be to: - make each property access's base effect free so metal codegen can emit - make the leftmost base of RMW effect free so we can emit the "leftValue()" twice
and add back all the property resolver tests that make sense in this new world
also we'll want the "high zombie finder" to have some new rules
Created attachment 378121 [details] WIP it begins
Created attachment 378142 [details] WIP it seems to work.
Created attachment 378151 [details] WIP I think it's done, I just want to write some more tests
Created attachment 378251 [details] patch
Comment on attachment 378251 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=378251&action=review r=me > Source/WebCore/ChangeLog:9 > + The goal of the new property resolver phase is two allow two things: two allow => to allow > Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:90 > + while (true) { This pattern of ``` while(true) { .. chain.append(current); if (is<AST::PropertyAccessExpression>(current.base())) continue; Tons of code that only executes once break; } ``` is not super readable. It is probably not worth modifying now, but I am curious, why not just use the loop to accumulate the chain: ``` while (is<AST::PropertyAccessExpression>(current.base())) { append to chain current = *currentPtr; } tons of code that manipulate current but are now outside of the loop ``` > Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:245 > + RELEASE_ASSERT(m_variables.isEmpty()); There is no guarantee that the WTFMove later on will empty-out m_variables. I think you need to either empty it by hand, or more simply to construct a different PropertyResolver for each function definition in resolveProperties. > Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLReadModifyWriteExpression.h:63 > + ReadModifyWriteExpression(CodeLocation location, UniqueRef<Expression> leftValue, UniqueRef<VariableDeclaration> oldValue, UniqueRef<VariableDeclaration> newValue) I am unclear on whether it is better to pass UniqueRef<Expression> or UniqueRef<Expression>&& here.
Comment on attachment 378251 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=378251&action=review >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:90 >> + while (true) { > > This pattern of > ``` > while(true) { > .. > chain.append(current); > if (is<AST::PropertyAccessExpression>(current.base())) > continue; > Tons of code that only executes once > break; > } > ``` > is not super readable. It is probably not worth modifying now, but I am curious, why not just use the loop to accumulate the chain: > ``` > while (is<AST::PropertyAccessExpression>(current.base())) { > append to chain > current = *currentPtr; > } > tons of code that manipulate current but are now outside of the loop > ``` fixed. >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:245 >> + RELEASE_ASSERT(m_variables.isEmpty()); > > There is no guarantee that the WTFMove later on will empty-out m_variables. > I think you need to either empty it by hand, or more simply to construct a different PropertyResolver for each function definition in resolveProperties. that's how move constructor of Vector works. It swaps with empty vector, so it should be emptied out. >> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLReadModifyWriteExpression.h:63 >> + ReadModifyWriteExpression(CodeLocation location, UniqueRef<Expression> leftValue, UniqueRef<VariableDeclaration> oldValue, UniqueRef<VariableDeclaration> newValue) > > I am unclear on whether it is better to pass UniqueRef<Expression> or UniqueRef<Expression>&& here. I prefer what I did here since it expressed the fact that this function now has ownership over the UniqueRef. I'm not a fan of passing around rvalue refs since it doesn't actually guarantee that ownership will be transferred.
thanks for the review. Landed in: https://trac.webkit.org/changeset/249630/webkit
landed watchOS build fix in: https://trac.webkit.org/changeset/249664/webkit
<rdar://problem/55196672>
This patch turns out to create comma expressions on the left-side of assignments. It is not something which could otherwise happen. Do we want to support it, or to change this pass?