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

Thu Oct 1 20:03:39 PDT 2020

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

Sorry for the delay. Took another run through. Thanks.

================
Comment at: mlir/include/mlir/IR/OpBase.td:2364
+
+  // Short summary of the type
+  string summary = ?;
----------------
nit: Please add proper punctuation to the end of comments.

There are many cases within the revision still.

================
Comment at: mlir/include/mlir/IR/OpBase.td:2374
+  string storageNamespace = "detail";
+  // Should we generate the storage class? (Or use an existing one?)
+  bit genStorageClass = 1;
----------------
nit: Phrase the comments as statements instead of questions.

================
Comment at: mlir/include/mlir/IR/OpBase.td:2440
+    llvm::SmallVector<}] # arrayOf # [{, 4> tmpFields;
+    for (size_t i = 0, e = $_self.size(); i < e; i++) {
+      tmpFields.push_back($_self[i].allocateInto($_allocator));
----------------
nit: Drop trivial braces, and use pre-increment.

================
Comment at: mlir/include/mlir/IR/OpBase.td:2444
+    $_dst = $_allocator.copyInto(ArrayRef<}] # arrayOf # [{>(tmpFields));
+    }];
+}
----------------
The formatting is still a bit off.

================
Comment at: mlir/include/mlir/TableGen/TypeDef.h:132
+template <typename T>
+void TypeDef::getParametersAs(SmallVectorImpl<T> &parameters,
+                              llvm::function_ref<T(TypeParameter)> map) const {
----------------
Seems like TypeDef could just provide a range of it's parameters and callers of this could just you llvm::map_range instead.

================
Comment at: mlir/lib/TableGen/TypeDef.cpp:69
+    size_t numParams = parametersDag->getNumArgs();
+    for (unsigned i = 0; i < numParams; i++) {
+      parameters.push_back(TypeParameter(parametersDag, i));
----------------
nit: Drop trivial braces here and throughout the revision.

================
Comment at: mlir/lib/TableGen/TypeDef.cpp:130
+      "Parameters DAG arguments must be either strings or defs "
+      "which inherit from TypeParameter\n");
+}
----------------
Drop the newline here.

================
Comment at: mlir/lib/TableGen/TypeDef.cpp:163
+    return getCppType();
+  } else {
+    llvm::errs() << "Parameters DAG arguments must be either strings or defs "
----------------
Don't use else after return.

================
Comment at: mlir/lib/TableGen/TypeDef.cpp:164
+  } else {
+    llvm::errs() << "Parameters DAG arguments must be either strings or defs "
+                    "which inherit from TypeParameter\n";
----------------
Should this be a fatal error instead?

================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:20
+
+namespace mlir {
+
----------------
Namespaces in source files should only really be used for class declarations. Everything else should be in the top-level scope.

================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:28
+  if (parser.parseKeyword(&signStr))
+    return mlir::failure();
+  if (signStr.compare_lower("u") || signStr.compare_lower("unsigned"))
----------------
nit: Drop all of the `mlir::` in this file, and throughout the revision.

================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:36
+  else {
+    parser.emitError(loc, "expected signed, unsigned, or none");
+    return mlir::failure();
----------------
nit: Return the error directly.

================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:63
+  SmallVector<int, 4> arrayOfInts;
+  if (parser.parseLess())
+    return Type();
----------------
nit: Merge all of these together using `||`

================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:93
+  auto intArray = getArrayOfInts();
+  for (size_t idx = 0; idx < intArray.size(); idx++) {
+    printer << intArray[idx];
----------------
nit: Use llvm::interleaveComma here instead.

================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:101
+
+bool operator==(const FieldInfo &a, const FieldInfo &b) {
+  return a.name == b.name && a.type == b.type;
----------------
The definitions of methods in header files should be fully qualified `::` and not placed in a namespace in the .cpp.

================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.h:27

+struct FieldInfo {
+public:
----------------
Please add comments for this class and it's usages.

================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.h:28
+struct FieldInfo {
+public:
+  StringRef name;
----------------
This public seems unnecessary.

================
Comment at: mlir/tools/mlir-tblgen/OpDocGen.cpp:202
+       << "`\n";
+  } else {
+    os << "\nSyntax:\n\n```\n!" << td.getDialect().getName() << "."
----------------
nit: Prefer early return instead.

================
Comment at: mlir/tools/mlir-tblgen/OpDocGen.cpp:205
+       << td.getMnemonic() << "<\n";
+    for (auto *it = parameters.begin(); it < parameters.end(); it++) {
+      os << "  " << it->getSyntax();
----------------
nit: Unless the end loop iterator changes, cache it.

================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:83
+  SmallVector<std::string, 4> parameters;
+  if (prependComma)
+    parameters.push_back("");
----------------
The name of this parameter doesn't seem to match what you are doing here.

================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:85
+    parameters.push_back("");
+  typeDef.getParametersAs<std::string>(parameters, [](auto parameter) {
+    return (parameter.getCppType() + " " + parameter.getName()).str();
----------------
Could you just format the parameters into a llvm::raw_string_ostream instead of constructing separate strings?

================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:98
+static std::string constructParametersInitializers(TypeDef &typeDef) {
+  SmallVector<std::string, 4> parameters;
+  typeDef.getParametersAs<std::string>(parameters, [](auto parameter) {
----------------
Same comment as above.

================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:134
+                                        {1}::{2}> {{
+public:
+    /// Inherit some necessary constructors from 'TypeBase'.
----------------
This looks like the indent is off.

================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:186
+  if (auto mnenomic = typeDef.getMnemonic()) {
+    os << "    static ::llvm::StringRef getMnemonic() { return \"" << mnenomic
+       << "\"; }\n";
----------------
Do we need to generate this method? Seems like the uses of it by the generator could just inline the string instead.

================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:197
+
+    for (auto parameter : parameters) {
+      SmallString<16> name = parameter.getName();
----------------
nit: Spell out auto here, also use a reference instead of by-value.

================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:222
+    // well known print/parse dispatch function declarations
+    os << "  ::mlir::Type generatedTypeParser(::mlir::MLIRContext* ctxt, "
+          "::mlir::DialectAsmParser& parser, ::llvm::StringRef mnenomic);\n";
----------------
Seems like this would be something only generated in the source file and marked as static.

================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:274
+  struct {1} : public ::mlir::TypeStorage {{
+      {1} ({2})
+          : {3} {{ }
----------------
This looks over indented, should be 2 spaces not 4.

================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:293
+      static {0} *construct(::mlir::TypeStorageAllocator &allocator,
+                                          const KeyTy &key) {{
+)";
----------------
This looks misaligned.

================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:311
+  auto fmtCtxt = FmtContext().addSubst("_allocator", "allocator");
+  for (auto parameter : parameters) {
+    auto allocCode = parameter.getAllocator();
----------------
nit: Spell out auto here and don't use a by-value iterator variable.

================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:348
+  for (size_t i = 0; i < parameters.size(); i++) {
+    os << llvm::formatv("      auto {0} = std::get<{1}>(key);\n",
+                        parameters[i].getName(), i);
----------------
Should these be `auto &`instead?

================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:373
+                        typeDef.getStorageClassName());
+    // I want C++17's unboxing!!!
+    for (size_t i = 0; i < parameters.size(); i++) {
----------------
Please remove all of the personal comments from the revision.

================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:381
+      return mlir::failure();
+    // return an allocated copy
+    os << llvm::formatv(typeDefStorageClassConstructorReturn,
----------------
nit: Comments should be complete sentences. This applies throughout the revision.

================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:420
+  if (typeDef.genAccessors()) {
+    for (auto parameter : parameters) {
+      SmallString<16> name = parameter.getName();
----------------
nit: Same comment about auto here as above.

================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:481
+
+  } // typeDef.getMnemonic()
+  return mlir::success();
----------------
Stray comment?

================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:505
+
+  os << "bool generatedTypePrinter(::mlir::Type type, "
+        "::mlir::DialectAsmPrinter& "
----------------
Can we use a LogicalResult return instead?

================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:513
+      os << llvm::formatv("    .Case<{0}::{1}>([&](::mlir::Type t) {{ "
+                          "t.dyn_cast<{0}::{1}>().print(printer); })\n",
+                          typeDef.getDialect().getCppNamespace(),
----------------
Can we avoid generating print/parse on the types themselves? We should try to keep as much internal details hidden away as possible.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86904