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

[Lei Zhang via Phabricator via llvm-commits]

antiagainst added inline comments.
Herald added a subscriber: mgester.


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp:664
+
+static LogicalResult verifyRegionArgResultAttribute(Location loc,
+                                                    NamedAttribute attribute,
----------------
mravishankar wrote:
> "verifyRegionArgResultAttribute" is misleading. What is intended by ArgResult? How about just "verifyRegionArgAttribute"
Changed to `verifyRegionAttribute` and added comments to avoid the confusion.


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp:696
+    NamedAttribute attribute) {
+  return verifyRegionArgResultAttribute(op->getLoc(), attribute,
+                                        /*forArg=*/false);
----------------
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. :) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72062