[PATCH] D71930: [mlir][spirv] Add basic definitions for supporting availability

[Mahesh Ravishankar via Phabricator via llvm-commits]

mravishankar requested changes to this revision.
mravishankar added a comment.
This revision now requires changes to proceed.

The SPIR-V part looks fine to me. I have no major comments on the tblgen part. Maybe get some one with more knowledge to review it.



================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVBase.td:49
+//===----------------------------------------------------------------------===//
+// Op availability definitions
+//===----------------------------------------------------------------------===//
----------------
I am assuming these will move into a different file some time. Can we put them in a different td file for separation?


================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVBase.td:165
+    extensions for the implementing SPIR-V operation. The returned value
+    is a neted vector whose element is `mlir::spirv::Extension`s. The outer
+    vector's elements (which are vectors) should be interpreted as conjunction
----------------
nit : neted -> nested


================
Comment at: mlir/test/Dialect/SPIRV/TestAvailability.cpp:19
+struct TestAvailability : public FunctionPass<TestAvailability> {
+  void runOnFunction() override;
+};
----------------
Should this be a module pass? Do we need to look at ops at "module-level" to arrive at capabilities?


================
Comment at: mlir/tools/mlir-tblgen/SPIRVUtilsGen.cpp:1
 //===- SPIRVSerializationGen.cpp - SPIR-V serialization utility generator -===//
 //
----------------
Is there a reason to not have them in a separate file. The additions are completely independent here right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71930