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

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

Thanks for driving this!

Started making my way through it.



================
Comment at: mlir/include/mlir/IR/OpBase.td:2355
+
+// Define a new type belonging to a dialect and called 'name'
+class TypeDef<Dialect dialect, string name> {
----------------
nit: Please add proper punctuation to the end of comments.


================
Comment at: mlir/include/mlir/IR/OpBase.td:2357
+class TypeDef<Dialect dialect, string name> {
+  Dialect owningDialect = dialect;
+  string cppClassName = name # "Type";
----------------
nit: I would swap the names here:

Dialect dialect = owningDialect;


================
Comment at: mlir/include/mlir/IR/OpBase.td:2394
+  // Extra code to include in the class declaration
+  code extraDecls = [{}];
+}
----------------
Please use the same name for this field as the other ODS classes; `extraClassDeclaration`.


================
Comment at: mlir/include/mlir/IR/OpBase.td:2400
+class TypeMember<string type, string desc> {
+  code allocator = ?;
+  string cppType = type;
----------------
Can you add comments to these?


================
Comment at: mlir/include/mlir/IR/OpBase.td:2427
+    llvm::SmallVector<}] # arrayOf # [{, 4> tmpFields($_self.size());
+    for (size_t i=0; i<$_self.size(); i++) {
+      tmpFields[i] = $_self[i].allocateInto($_allocator);
----------------
nit: There are a few format issues in this code block.


================
Comment at: mlir/include/mlir/TableGen/CodeGenHelpers.h:26
+public:
+  inline IfDefScope(llvm::StringRef name, llvm::raw_ostream &os)
+      : name(name), os(os) {
----------------
nit: Only specify `inline` on methods if you have a good reason to.


================
Comment at: mlir/include/mlir/TableGen/CodeGenHelpers.h:41
+public:
+  inline NamespaceEmitter(raw_ostream &os, const Dialect &dialect) : os(os) {
+    if (!dialect)
----------------
nit: Same comments on inline here and throughout. 


================
Comment at: mlir/include/mlir/TableGen/CodeGenHelpers.h:41
+public:
+  inline NamespaceEmitter(raw_ostream &os, const Dialect &dialect) : os(os) {
+    if (!dialect)
----------------
rriddle wrote:
> nit: Same comments on inline here and throughout. 
Seems like this could take the fully qualified cpp namespace string instead of a dialect.


================
Comment at: mlir/include/mlir/TableGen/TypeDef.h:1
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
This comment block looks incorrect, can you look at other files for examples?


================
Comment at: mlir/include/mlir/TableGen/TypeDef.h:22
+namespace llvm {
+class Record;
+class DagInit;
----------------
Are these necessary given that you include Record.h?


================
Comment at: mlir/include/mlir/TableGen/TypeDef.h:61
+
+  // I don't remember what this is for or how it'd work...
+  bool hasStorageCustomConstructor() const;
----------------
nit: Can you add a better comment here?


================
Comment at: mlir/include/mlir/TableGen/TypeDef.h:101
+
+  // Compares two dialects by comparing the names of the dialects.
+  bool operator<(const TypeDef &other) const;
----------------
This comment and a few others look like they need to be updated.


================
Comment at: mlir/include/mlir/TableGen/TypeDef.h:111
+
+class TypeMember {
+public:
----------------
This class needs some documentation.


================
Comment at: mlir/include/mlir/TableGen/TypeDef.h:129
+void TypeDef::getMembersAs(SmallVectorImpl<T> &members,
+                           std::function<T(TypeMember)> map) const {
+  auto membersDag = def->getValueAsDag("members");
----------------
nit: Use llvm::function_ref instead of std::function if you don't need to store the functor.


================
Comment at: mlir/include/mlir/TableGen/TypeDefGenHelpers.h:2
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
----------------
Do we need this file for this commit? This looks like it is only used for the testing and I'd rather leave parsing/printer utilities separately.


================
Comment at: mlir/lib/TableGen/TypeDef.cpp:23
+  return Dialect(
+      dyn_cast<llvm::DefInit>(def->getValue("owningDialect")->getValue())
+          ->getDef());
----------------
`dyn_cast` may return null, use cast if you know that it is of the type you expect.


================
Comment at: mlir/lib/TableGen/TypeDef.cpp:33
+bool TypeDef::hasDescription() const {
+  const auto *s = def->getValue("description");
+  return s != nullptr && isa<llvm::StringInit>(s->getValue());
----------------
nit: Can you spell out auto here, the type isn't obvious.


================
Comment at: mlir/lib/TableGen/TypeDef.cpp:65
+  auto *membersDag = def->getValueAsDag("members");
+  if (membersDag != nullptr)
+    for (unsigned i = 0; i < membersDag->getNumArgs(); i++)
----------------
nit: Add braces around the if here, the body isn't trivial.


================
Comment at: mlir/lib/TableGen/TypeDef.cpp:66
+  if (membersDag != nullptr)
+    for (unsigned i = 0; i < membersDag->getNumArgs(); i++)
+      members.push_back(TypeMember(membersDag, i));
----------------
nit: Cache the end iterator of for loops unless you rely on the value changing during the loop.


================
Comment at: mlir/lib/TableGen/TypeDef.cpp:71
+  auto *membersDag = def->getValueAsDag("members");
+  if (membersDag == nullptr)
+    return 0;
----------------
`return membersDag ? membersDag->getNumArgs() : 0;`?


================
Comment at: mlir/lib/TableGen/TypeDef.cpp:85
+      "Record `" + def->getName() +
+          "', field `printer' does not have a code initializer!");
+}
----------------
The error message here seems off.


================
Comment at: mlir/lib/TableGen/TypeDef.cpp:129
+    return llvm::Optional<StringRef>();
+  } else if (auto *typeMember = dyn_cast<llvm::DefInit>(memberType)) {
+    auto *code = typeMember->getDef()->getValue("allocator");
----------------
nit: Don't use else after return.


================
Comment at: mlir/lib/TableGen/TypeDef.cpp:141
+  } else {
+    llvm::errs() << "Members DAG arguments must be either strings or defs "
+                    "which inherit from TypeMember\n";
----------------
PrintFatalError? Same below.


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

https://reviews.llvm.org/D86904