[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/SPIRVLowering.h:77
   /// Creates a SPIR-V conversion target for the given target environment.
-  static std::unique_ptr<SPIRVConversionTarget> get(TargetEnvAttr targetEnv,
-                                                    MLIRContext *context);
+  static std::unique_ptr<SPIRVConversionTarget> get(TargetEnvAttr targetAttr);
 
----------------
mravishankar wrote:
> Added a comment on the previous CL in this stack, but just mirroring here. I think its would be good to have a `get` method which will set the default target env. Its seems more ergonomic to me.
Replied in the previous CL.


================
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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76242