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

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:420
+/// This class manages the state of SSA value names.
+class SSANameState {
 public:
----------------
antiagainst wrote:
> 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?
I don't think this is necessary. 4 billion values is more than enough for a long time.


================
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 };
----------------
antiagainst wrote:
> With or without? I thought it should be with here?
Done. Thanks.


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:691
+  Operation *owner = result->getOwner();
+  if (owner->getNumResults() == 1)
+    return;
----------------
antiagainst wrote:
> Do we need to update `lookupValue` for this case?
Nope, only necessary if 'result' is in the middle of a result group, otherwise 'lookupValue' is already the head of the result group.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72293