[PATCH] D86904: [mlir] Support for defining Types in tblgen

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

Really great stuff, I'm glad you've been pushing this! Mostly nits left for the code, it's enough that we can just iterate further in tree. Can you document all of this cool new stuff? Either in docs/OpDefinitions or a new docs/TypeDefinitions?



================
Comment at: mlir/include/mlir/TableGen/TypeDef.h:71
+  // supposed to auto-generate them.
+  llvm::Optional<StringRef> getMnemonic() const;
+
----------------
nit: Can we remove the llvm:: on these as well?


================
Comment at: mlir/include/mlir/TableGen/TypeDef.h:93
+  // Get the code location (for error printing).
+  llvm::ArrayRef<llvm::SMLoc> getLoc() const;
+
----------------
Is the llvm:: here necessary?


================
Comment at: mlir/lib/TableGen/TypeDef.cpp:152
+    return stringType->getValue();
+  else if (auto *typeParameter = dyn_cast<llvm::DefInit>(parameterType)) {
+    const auto *syntax = typeParameter->getDef()->getValue("syntax");
----------------
Please drop else after a return.


================
Comment at: mlir/lib/TableGen/TypeDef.cpp:154
+    const auto *syntax = typeParameter->getDef()->getValue("syntax");
+    if (syntax != nullptr && isa<llvm::StringInit>(syntax->getValue()))
+      return dyn_cast<llvm::StringInit>(syntax->getValue())->getValue();
----------------
nit: Drop the `!= nullptr`.


================
Comment at: mlir/test/lib/Dialect/Test/TestTypeDefs.td:118
+    $_printer << "struct" << "<";
+    for (size_t i=0; i<getImpl()->fields.size(); i++) {
+    const auto& field = getImpl()->fields[i];
----------------
Please cache the end iterator of loops if the size doesn't change.


================
Comment at: mlir/test/lib/Dialect/Test/TestTypeDefs.td:119
+    for (size_t i=0; i<getImpl()->fields.size(); i++) {
+    const auto& field = getImpl()->fields[i];
+    $_printer << "{" << field.name << "," << field.type << "}";
----------------
The formatting here seems off.


================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:36
+    result = TestIntegerType::SignednessSemantics::Signless;
+  else {
+    return parser.emitError(loc, "expected signed, unsigned, or none");
----------------
nit: Drop trivial braces here. Also if you use braces on one if/else, please use it for all of them.


================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:87
+
+namespace mlir {
+
----------------
Please only use namespaces in source files for classes.


================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:105
+    Location loc, TestIntegerType::SignednessSemantics ss, unsigned int width) {
+
+  if (width > 8)
----------------
nit: Drop the newline here.


================
Comment at: mlir/tools/mlir-tblgen/OpDocGen.cpp:273
+    os << "## Type definition\n\n";
+    for (const TypeDef &td : typeDefs) {
+      emitTypeDefDoc(td, os);
----------------
nit: Drop trivial braces here.


================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:57
+    dialectName = (*dialects.begin()).getName();
+  } else if (selectedDialect.getNumOccurrences() == 1)
+    dialectName = selectedDialect.getValue();
----------------
Please use braces on every else if they are used on one.


================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:63
+
+  for (auto typeDef : defs)
+    if (typeDef.getDialect().getName().equals(dialectName))
----------------
nit: `auto` -> `const TypeDef &`


================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:82
+    retStream << ", ";
+  for (auto it = parameters.begin(), e = parameters.end(); it < e; ++it) {
+    retStream << it->getCppType() << " " << it->getName();
----------------
llvm::interleaveComma?


================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:99
+    retStream << it->getName() << "(" << it->getName() << ")";
+    if (it < parameters.end() - 1)
+      retStream << ", ";
----------------
Same here?


================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:309
+      fmtCtxt.addSubst("_dst", parameter.getName());
+      auto fmtObj = tgfmt(*allocCode, &fmtCtxt);
+      os << "      ";
----------------
Can you stream the tgfmt directly into `os`?


================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:433
+        FmtContext().addSubst("_parser", "parser").addSubst("_ctxt", "ctxt");
+    auto fmtObj = tgfmt(*parserCode, &fmtCtxt);
+    fmtObj.format(os);
----------------
Can you stream it into `os` directly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86904