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

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/Tools/PDLL/ODS/Constraint.h:42
+  /// The name of the constraint.
+  std::string name;
+  /// A summary of the constraint.
----------------
jpienaar wrote:
> Are these std::string due to life time concerns?
Yeah, in a follow up I intend to make the summary/description stuff all optional.


================
Comment at: mlir/include/mlir/Tools/PDLL/ODS/Context.h:42
+
+  /// Register a new type constraint with the context. Returns the inserted
+  /// constraint.
----------------
jpienaar wrote:
> What happens if a constraint by that name already exists?
Just returns the previous one. Added in asserts that the info is the same on duplicate insertion.


================
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);
+
----------------
jpienaar wrote:
> 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?
Constraints are self-contained, and don't really need any mutation after insertion. Dialects can have additional constructs inserted (operations/etc). We could make the constraints non-const on return, but there isn't a reason to right now.


================
Comment at: mlir/include/mlir/Tools/PDLL/ODS/Context.h:65
+  /// Print the contents of this context to the provided stream.
+  void print(raw_ostream &os) const;
+
----------------
jpienaar wrote:
> Contrary to MLIR IR side is there any expectation that this is useful beyond debugging?
Not really. Printing here is really only for debugging/testing.


================
Comment at: mlir/include/mlir/Tools/PDLL/ODS/Operation.h:55
+  /// The ODS name of the attribute.
+  std::string name;
+
----------------
jpienaar wrote:
> Same question wrt string vs stringref here.
Yep, this is for lifetime issues.


================
Comment at: mlir/include/mlir/Tools/PDLL/ODS/Operation.h:115
+
+/// This class represents an opaque ODS representation of a specific operation.
+/// This includes all of the information necessary for use by the PDL frontend
----------------
jpienaar wrote:
> What does opaque here signify?
Just dropped all of the `opaque`.


================
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) {
----------------
jpienaar wrote:
> And here adds don't return anything (these feel inconsistent to have same names/prefixes enable different things)
Keeping these as `add` now that the functions returning things are called "insert".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119900