[PATCH] D119900: [PDLL] Add support for tablegen includes and importing ODS information
Jacques Pienaar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 28 18:59:20 PST 2022
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
More information about the llvm-commits
mailing list