[PATCH] D72062: [mlir][spirv] Fix shader ABI attribute prefix and add verification

[Lei Zhang via Phabricator via llvm-commits]

antiagainst marked 2 inline comments as done.
antiagainst added inline comments.


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp:696
+    NamedAttribute attribute) {
+  return verifyRegionArgResultAttribute(op->getLoc(), attribute,
+                                        /*forArg=*/false);
----------------
mravishankar wrote:
> antiagainst wrote:
> > mravishankar wrote:
> > > I am not sure it is well defined to have a InterfaceVarABIAttr for the result. So far the GPU to SPIR-V lowering assumes that entry point functions have no return values, and the LowerABIAttributesPass does not handle ABI attributes on the results. I tried to add that, but wasnt sure about the semantics there. So better to not handle it, and maybe error out when such attribute is specified on the result.
> > What are the problems you see for attaching InterfaceVarABIAttr to a result? Isn't the semantics of it a write-only buffer?
> > 
> > In the verification code implemented by this CL, I'm intentionally not checking which op the attribute is attached to so that we can allow "carrying over" these attributes on some intermediate state in the lowering process. (For example somebody might want to create an intermediate function op of some sort maybe.) So the verification here only focuses on the attributes themselves; not really paying attention to the op or region arg/result index/position, etc. I get your points that we are not supporting InterfaceVarABIAttr in GPU lowering yet but this still seems a step forward that is nice to have: previously we don't verify these attributes at all, now we at least make sure they are correct internally. :) 
> What would the function look like
> 
> func(%arg0 : memref<..>) -> memref<..> { ... }
> 
> This suggests that you are "returning" a memref, which makes ownership of that memref tricky to define. You could say that the lowering will convert this function into
> 
> func(%arg0 : memref<...>, %arg1 : memref<...>) {...}
> 
> and then say %arg1 is for the result. All this needs to be handled in the LowerABIAttributesPass. I was waiting for having a realistic use case for this. Currently when lowering from GPU dialect to SPIR-V, the entry function has void return type. So would rather have the verifier return error on this (cause its not implemented).
Right, it depends on the definition of the op and its semantics. It might not make sense for func given that we have a relatively clear definition and understanding. I could have my funky op that uses results for some other purposes. Not advocating that we should intentionally support such cases; it's just that IMO that is a separate concern than verifying the attribute itself: it's more of resulting from the op's semantics than the spv.* attributes' semantics.

This is just another incarnation of the open-vs-closed trade-off we are facing. :) Typically I'd be very worried if being open means we can have many unexpected ways to break. Then I'd prefer to enumerate what we have at hand and support them explicitly and go from there to extend as needed. But for this one it seems it's not gonna bite us that much (and whatever unexpected places it gets attached to have some minimal verification at least). So I'd think a little openness does not hurt that much. :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72062/new/

https://reviews.llvm.org/D72062