[PATCH] D76242: [mlir][spirv] Plumbing target environment into type converter

[Lei Zhang via Phabricator via llvm-commits]

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


================
Comment at: mlir/include/mlir/Dialect/SPIRV/TargetAndABI.h:54
+  TargetEnvAttr targetAttr;
+  llvm::SmallSet<Extension, 4> givenExtensions;    /// Allowed extensions
+  llvm::SmallSet<Capability, 8> givenCapabilities; /// Allowed capabilities
----------------
mravishankar wrote:
> antiagainst wrote:
> > mravishankar wrote:
> > > These might cause subtle issues later  on. Can we make this a SetVector?
> > But what we want here is just a set. We don't care about the order. What problems do you think will have with SmallSet?
> For now this is not iterated over. But there is no gaurantee later on and is easy to miss. Dont see too much advantage of using SmallSet vs SetVector, and just prefer to use the latter.
Yeah I think we can re-evaluate this later when we want to loop over the set. Likely we will not have that need; I'd assume querying whether some extensions/capabilities are allowed is all what we need for using the target environment. But you never know. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76242