[PATCH] D119900: [PDLL] Add support for tablegen includes and importing ODS information

[Jacques Pienaar via Phabricator via llvm-commits]

jpienaar accepted this revision.
jpienaar added inline comments.
This revision is now accepted and ready to land.


================
Comment at: mlir/include/mlir/Tools/PDLL/ODS/Constraint.h:1
+//===- Constraint.h - MLIR PD:L ODS Constraints -----------------*- C++ -*-===//
+//
----------------
PDLL instead of PD:L


================
Comment at: mlir/include/mlir/Tools/PDLL/ODS/Constraint.h:7
+//
+//===----------------------------------------------------------------------===//
+
----------------
(PDLL ODS constraints is such a mouthful that it may make sense to add file header comment)


================
Comment at: mlir/include/mlir/Tools/PDLL/ODS/Context.h:42
+
+  /// Register a new type constraint with the context. Returns the inserted
+  /// constraint.
----------------
What happens if a constraint by that name already exists?


================
Comment at: mlir/include/mlir/Tools/PDLL/ODS/Context.h:48
+  /// Register a new dialect with the context. Returns the inserted dialect.
+  Dialect &addDialect(StringRef name);
+
----------------
The above have add being const reference return, while here add is for mutable and lookup for non-mutable without insertion. What mutation is expected here/why isn't that expected above?


================
Comment at: mlir/include/mlir/Tools/PDLL/ODS/Context.h:57
+  /// operation already existed).
+  std::pair<Operation *, bool> addOperation(StringRef name, StringRef summary,
+                                            StringRef desc, SMLoc loc);
----------------
insertOperation? (This is why your comment even says :-)). Up to you. 


================
Comment at: mlir/include/mlir/Tools/PDLL/ODS/Dialect.h:36
+  /// operation already existed).
+  std::pair<Operation *, bool> addOperation(StringRef name, StringRef summary,
+                                            StringRef desc, llvm::SMLoc loc);
----------------
Ditto


================
Comment at: mlir/include/mlir/Tools/PDLL/ODS/Dialect.h:43
+
+  /// Return a range of all of the registered operations.
+  const llvm::StringMap<std::unique_ptr<Operation>> &getOperations() const {
----------------
Out dated comment? (The name makes more sense to me with this comment)


================
Comment at: mlir/include/mlir/Tools/PDLL/ODS/Operation.h:55
+  /// The ODS name of the attribute.
+  std::string name;
+
----------------
Same question wrt string vs stringref here.


================
Comment at: mlir/include/mlir/Tools/PDLL/ODS/Operation.h:71
+
+/// This class represents an opaque ODS representation of a specific operation
+/// operand or result. This includes the name, variable length flags, and more.
----------------
"represents a ... representation" feels off


================
Comment at: mlir/include/mlir/Tools/PDLL/ODS/Operation.h:124
+  /// Add an attribute to this operation.
+  void addAttribute(StringRef name, bool optional,
+                    const AttributeConstraint &constraint) {
----------------
And here adds don't return anything (these feel inconsistent to have same names/prefixes enable different things)


================
Comment at: mlir/lib/Tools/PDLL/ODS/Context.cpp:102
+    for (const Operation *op : sortMapByName(dialect->getOperations())) {
+      os << "    Operation `" << op->getName() << "` {\n";
+
----------------
Could we use the scoped & indented ostream emitter? (Seems would make it more less likely to forget a space)


================
Comment at: mlir/lib/Tools/PDLL/Parser/Parser.cpp:716
+  // Use a temporary buffer with a nested include file to let TableGen handle
+  // finding the location of and opening the include file.
+  std::string tempBuffer = llvm::formatv("include \"{0}\"\n", filename).str();
----------------
Sneaky :-) And include paths are not an issue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119900