[PATCH] D74783: [mlir][ODS] Add support for specifying the successors of an operation.

[Jacques Pienaar via Phabricator via llvm-commits]

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

NIce



================
Comment at: mlir/lib/TableGen/Operator.cpp:311
+  auto *successorsOp = dyn_cast<DefInit>(successorsDag->getOperator());
+  if (!successorsOp || successorsOp->getDef()->getName() != "successor") {
+    PrintFatalError(def.getLoc(),
----------------
Where does/could 'successorsOp->getDef()->getName() != "successor"' happen?


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:1346
+    if (!successor.name.empty())
+      name += std::string(formatv(" ('{0}')", successor.name));
+
----------------
Wouldn't formatv(...).str() also give this?

Feels a little weird to have a variable called name with spaces ...


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:1354
+
+    body << formatv("  if (!({0})) {\n    "
+                    "return emitOpError(\"successor {1} failed to verify "
----------------
Why not move this up and then you can emit the name directly in the body rather than concatting string above?


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:1401
+    // operand size traits currently check all operands.
+    opClass.addTrait("OpTrait::VariadicOperands");
+  } else if (numVariadicOperands != 0) {
----------------
It is a little bit difficult to correlate variadic operands with successors, could you explain that here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74783