[PATCH] D72293: [mlir] NFC: Move the state for managing SSA value names out of OperationPrinter and into a new class SSANameState.

[Lei Zhang via Phabricator via llvm-commits]

antiagainst accepted this revision.
antiagainst added a comment.
This revision is now accepted and ready to land.

Nice! I assume the changes for the printers are NFC so didn't pay much attention there. Just a few nits.



================
Comment at: mlir/lib/IR/AsmPrinter.cpp:420
+/// This class manages the state of SSA value names.
+class SSANameState {
 public:
----------------
I see we are using `unsigned` id values. Not sure whether it will be enough in the future. But to make it easier to extend, what about defining a `using IdType = unsigned` or something for it?


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:422
 public:
-  explicit ModuleState(MLIRContext *context) : interfaces(context) {}
+  /// A sentinal value used for values without names set.
+  enum : unsigned { NameSentinel = ~0U };
----------------
With or without? I thought it should be with here?


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:428
 
-  /// Get an instance of the OpAsmDialectInterface for the given dialect, or
-  /// null if one wasn't registered.
-  const OpAsmDialectInterface *getOpAsmInterface(Dialect *dialect) {
-    return interfaces.getInterfaceFor(dialect);
-  }
+  /// Print the ID for the given value to 'stream'.
+  void printValueID(Value value, bool printResultNo, raw_ostream &stream) const;
----------------
The name of this function is slightly misleading. It actually prints a symbol of the value; that symbol can either be a value ID or a value name. What about `printValueSymbol`?  (Or actually by value ID you mean value symbol? Either way, it would also be nice to explain the terms used at the very beginning: what is a value id mean, what is a value symbol (or whatever term).)


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:428
 
-  /// Get an instance of the OpAsmDialectInterface for the given dialect, or
-  /// null if one wasn't registered.
-  const OpAsmDialectInterface *getOpAsmInterface(Dialect *dialect) {
-    return interfaces.getInterfaceFor(dialect);
-  }
+  /// Print the ID for the given value to 'stream'.
+  void printValueID(Value value, bool printResultNo, raw_ostream &stream) const;
----------------
antiagainst wrote:
> The name of this function is slightly misleading. It actually prints a symbol of the value; that symbol can either be a value ID or a value name. What about `printValueSymbol`?  (Or actually by value ID you mean value symbol? Either way, it would also be nice to explain the terms used at the very beginning: what is a value id mean, what is a value symbol (or whatever term).)
Explain `printResultNo`?


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:432
+  /// Return the result groups registered by this operation, or empty if none
+  /// exist.
+  ArrayRef<int> getOpResultGroups(Operation *op);
----------------
Explain a bit more about the result `ArrayRef<int>`?


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:460
+  void getResultIDAndNumber(OpResult result, Value &lookupValue,
+                            int &lookupResultNo) const;
+
----------------
Wouldn't a `Optional<int>` be better here for `lookupResultNo`?


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:469
+
+  /// This is the value ID for each SSA value. If this returns ~0, then the
+  /// valueID has an entry in valueNames.
----------------
s/~0/NameSentinel/


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:475
+  /// This is a map of operations that contain multiple named result groups,
+  /// i.e. there may be multiple names for the results of the operation. The key
+  /// of this map are the result numbers that start a result group.
----------------
You mean the value of the map?


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:517
+
+  // If this is a reference to the result of a multi-result operation or
+  // operation, print out the # identifier and make sure to map our lookup
----------------
Redundant `or operation`?


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:586
+  // the same.
+  unsigned curValueID = nextValueID;
+  unsigned curArgumentID = nextArgumentID;
----------------
Do we have some existing RAII mechanism that we can use for this?


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:691
+  Operation *owner = result->getOwner();
+  if (owner->getNumResults() == 1)
+    return;
----------------
Do we need to update `lookupValue` for this case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72293