[PATCH] D73826: [mlir][Linalg] Provide a Tablegen backend to specify named Linalg ops declaratively

[Lei Zhang via Phabricator via llvm-commits]

antiagainst accepted this revision.
antiagainst added a comment.
This revision is now accepted and ready to land.

LGTM, but may want another pair of eyes to check the Linalg specific logic. :)



================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredInterface.td:1
+//===- LinalgStructuredInterface.td- Linalg StructuredIfce -*- tablegen -*-===//
+//
----------------
Super nit: Do we need to provide the "Linalg StructuredIfce" part with strange abbr.? Doesn't seem to provide anything to me. :)


================
Comment at: mlir/tools/mlir-tblgen/LinalgNamedOpsGen.cpp:30
+using namespace mlir;
+using namespace mlir::tblgen;
+
----------------
I'd prefer to have explicit `using` like the following that doing this.


================
Comment at: mlir/tools/mlir-tblgen/LinalgNamedOpsGen.cpp:92
+  for (const llvm::Record *r : defs) {
+    const auto *traitName = "mlir::OpTrait::linalg::NamedStructuredOpTraits";
+    auto opName = r->getName();
----------------
Nit: just use `char` to be clear here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73826