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

Fri Jan 10 10:13:01 PST 2020

rriddle added a comment.

What are the constraints here? Who is populating the target attribute on the module? How is this specified from a hypothetical front-end? I ask mainly because we are intending to have a general solution for "target" availability, that should(ideally) be a more general form of some the things introduced here.

================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVLowering.h:73
+
+  Version givenVersion;                            // SPIR-V version to target
+  llvm::SmallSet<Extension, 4> givenExtensions;    // Allowed extensions
----------------
Use `///` instead of trailing `//`.

================
Comment at: mlir/include/mlir/Dialect/SPIRV/TargetAndABI.td:32
 // 3) Storage class.
-def SPV_InterfaceVarABIAttr:
-    StructAttr<"InterfaceVarABIAttr", SPV_Dialect,
-               [StructFieldAttr<"descriptor_set", I32Attr>,
-                StructFieldAttr<"binding", I32Attr>,
-                StructFieldAttr<"storage_class", SPV_StorageClassAttr>]>;
+def SPV_InterfaceVarABIAttr: StructAttr<"InterfaceVarABIAttr", SPV_Dialect, [
+    StructFieldAttr<"descriptor_set", I32Attr>,
----------------
nit: space before the :

================
Comment at: mlir/include/mlir/Dialect/SPIRV/TargetAndABI.td:58
+]>;
+
+
----------------
nit: Remove the extra newline

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72256