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

[Mahesh Ravishankar via Phabricator via llvm-commits]

mravishankar requested changes to this revision.
mravishankar added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: arpith-jacob.

Thanks for fixing the naming!



================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp:664
+
+static LogicalResult verifyRegionArgResultAttribute(Location loc,
+                                                    NamedAttribute attribute,
----------------
"verifyRegionArgResultAttribute" is misleading. What is intended by ArgResult? How about just "verifyRegionArgAttribute"


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72062