[PATCH] D72256: [mlir][spirv] Properly support SPIR-V conversion target

[Lei Zhang via Phabricator via llvm-commits]

antiagainst marked an inline comment as done.
antiagainst added a comment.

Thanks for the review, Ben!!



================
Comment at: mlir/test/Dialect/SPIRV/target-env.mlir:22
+// Version: 1.0 (0), 1.1 (1), 1.2 (2), 1.3 (3), 1.4 (4)
+// Capability: Kernel (6), AtomicStorage (21), GroupNonUniformBallot (64),
+//             SubgroupBallotKHR (4423)
----------------
benvanik wrote:
> I think I get why the capability needs to be an int (as you wouldn't know the encoding of unknown capabilities in the compiler otherwise), but to improve the readability of the MLIR what about adding comments on the spv.target_env printer like `capabilities = [6, 7]}  // ["Foo", "Bar"]` for known ones? Would help for those of us who may want to ensure the IR is referencing the right caps but don't have the IDs memorized :)
Using numbers also helps when doing capability probing/comparison in conversion target. (We cannot do the same for extensions because extensions do not have a spec-defined number.) I don't think we can control how an attr is printed, unless to create a dialect-specific attribute. That is an improvement that can be deferred; I'll look into it later. :) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72256