3 options: Trapping Clamping Zero reads / writes do nothing
Gonna look at this today.
Planning on doing: "Zero reads / writes do nothing"
Created attachment 371908 [details] WIP Has a bunch of Myles's patches applied locally so I can test.
Created attachment 371912 [details] WIP
So the plan is: 1. The value stack in FunctionWriter turns into a stack of pairs: rvalues and lvalues. 2. All lvalues are pointers. 3. Anything that produces an lvalue must push a pointer to the stack. But not all things produce lvalues, so that field may be empty. However, all things that produce lvalues also produce rvalues. So, "*x = 42" works, but so does "foo(*x)". 4. Dereference just works, as dereference produces both an lvalue and rvalue. Dereference node's child must also be an lvalue. So we just forward that value along on the stack. For the rvalue, if we try to dereference nullptr, we just fill in zero bytes instead. 5. Assignment expressions check if the incoming lvalue is nullptr. If it is, we skip the assignment. 6. Then, operator&[] just returns nullptr on OOB. Then dereference works as expected, and assignment works as expected. So does "thread *foo = null; *foo = 42; *foo;". 7. MakePointerExpression just takes the last lvalue off the stack and returns it. 8. VariableReference will push both the variable value and a pointer to the variable.
I believe the patch accomplishes this, just need to write some tests.
<rdar://problem/51668853>
Created attachment 372013 [details] WIP
Created attachment 372074 [details] patch This is ready for review. It relies on the compute patch (which just got rolled out). So I will hold off on landing this until that re-lands.
Comment on attachment 372074 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=372074&action=review > Source/WebCore/platform/graphics/gpu/cocoa/GPUComputePipelineMetal.mm:97 > + if (!computeLibrary) > + NSLog(@"%@", error); I find these logs helpful during development, but I can remove them if that's preferred.
Comment on attachment 372074 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=372074&action=review It looks good overall, I've made a few comments inline. > Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:113 > + rhs->setTypeAnnotation(AST::LeftValue { AST::AddressSpace::Thread }); I don't get this, why would the right side of an assignment be a left-value? > Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:139 > + makePointerExpression->setTypeAnnotation(AST::RightValue()); I agree with this one on the other hand > Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:692 > + variableReference->setTypeAnnotation(AST::LeftValue { AST::AddressSpace::Thread }); // FIXME: https://bugs.webkit.org/show_bug.cgi?id=198169 Is this right? No it's not, I made readModifyWrite expressions right values in the spec (it gets super confusing otherwise, e.g. what would (x += y) = z do?) > Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:612 > + // FIXME: This needs to be made to work. It probably should be using the last leftValue too. I agree that it should be using the leftValue. > Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:84 > +String writeNativeFunction(AST::NativeFunctionDeclaration& nativeFunctionDeclaration, String& outputFunctionName, Intrinsics& intrinsics, TypeNamer& typeNamer, const char* memsetZeroFunctionName) What is the reasoning behind passing memsetZeroFunctionName as an argument? Is it not a global constant? >> Source/WebCore/platform/graphics/gpu/cocoa/GPUComputePipelineMetal.mm:97 >> + NSLog(@"%@", error); > > I find these logs helpful during development, but I can remove them if that's preferred. I'm happy to see logs, but maybe behind a !ASSERT_DISABLED ? So they will be free in release mode.
Comment on attachment 372074 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=372074&action=review >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:113 >> + rhs->setTypeAnnotation(AST::LeftValue { AST::AddressSpace::Thread }); > > I don't get this, why would the right side of an assignment be a left-value? Spoke to Robin offline, but commenting here for posterity: In the current implementation of the compiler, all VariableReferences are Lvalues, even if we don't use them as an lvalue. This patch makes this happen consistently everywhere. It simplifies how we generate code in metal. Alternatively, we could not rely on this, but metal code gen that produces Lvalues would have to branch to see if the current node is used in a lvalue context or not. I thought it'd be easier to just make this consistent everywhere. Now, the metal codegen will produce an lvalue irrespective of if the lvalue is uses. That's ok, because in the worst case, we just end up with unused variables, which LLVM will happily kill the various operations used to produce such Lvalues. >> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:84 >> +String writeNativeFunction(AST::NativeFunctionDeclaration& nativeFunctionDeclaration, String& outputFunctionName, Intrinsics& intrinsics, TypeNamer& typeNamer, const char* memsetZeroFunctionName) > > What is the reasoning behind passing memsetZeroFunctionName as an argument? Is it not a global constant? It's currently scoped to the class. I thought it is nicer to just pass it in. >>> Source/WebCore/platform/graphics/gpu/cocoa/GPUComputePipelineMetal.mm:97 >>> + NSLog(@"%@", error); >> >> I find these logs helpful during development, but I can remove them if that's preferred. > > I'm happy to see logs, but maybe behind a !ASSERT_DISABLED ? So they will be free in release mode. Well, we shouldn't ever reach this. If we do, it's a bug in the compiler. We could put it under !NDEBUG or something like that.
Comment on attachment 372074 [details] patch r=me
Created attachment 372127 [details] patch for landing
Attachment 372127 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:40: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 372128 [details] patch for landing
landed in: https://trac.webkit.org/changeset/246438/webkit
follow up landed in: https://trac.webkit.org/changeset/246439/webkit