[PATCH] D72294: [mlir] Refactor ModuleState into AsmState and expose it to users.

[Jacques Pienaar via Phabricator via llvm-commits]

jpienaar added a comment.
Herald added subscribers: liufengdb, aartbik.

Looks good on first walk through



================
Comment at: mlir/include/mlir/IR/AsmState.h:7
+//
+//===----------------------------------------------------------------------===//
+
----------------
File description? 


================
Comment at: mlir/include/mlir/IR/AsmState.h:26
+/// The IR should not be mutated in-between invocations using this state, and
+/// the IR being printed must not be at a higher level than the IR originally
+/// used to initialize this state. This means that if a child operation is
----------------
What does higher level mean here? It is a bit overloaded term and I think you have a very explicit meaning in mind.


================
Comment at: mlir/include/mlir/IR/AsmState.h:41
+  /// A pointer to allocated storage for the impl state.
+  std::unique_ptr<detail::AsmStateImpl> impl;
+};
----------------
So the lifetime of the AsmStateImpl is linked to this object, and unique_ptr is used to enable only forward declaring it above?


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:2201
+  // name and not the shadowed name.
+  state.getImpl().getSSANameState().printValueID(*this, /*printResultNo=*/true,
+                                                 os);
----------------
How is guaranteed that getImpl doesn't return null here? (e.g., verify that AsmState has been initialized)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72294