[PATCH] D81558: [NewPM] Introduce PreserveCFG check

[Fedor Sergeev via Phabricator via llvm-commits]

fedor.sergeev added a comment.

NewPM use seems to be fine.
A few relatively minor comments that I would like to see addressed.



================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:89
+// poisoned and no block pointer of the Graph is used.
+struct CFG {
+  struct BBGuard final : public CallbackVH {
----------------
Are  we ok with adding a generic 'CFG' class directly into llvm namespace by StandardInstrumentations.h?
If it is only for CFGChecker introduced here then either put it into Checker itself or into a nested namespace named appropriately
(e.g. cfg_checker_detail).

If you can see this class being generally useful then it deserves its own header/location.


================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:120-121
+private:
+  SmallVector<Optional<CFG>, 8> GraphStackBefore;
+  SmallVector<StringRef, 8> PassStack;
+
----------------
It seems that GraphStack and PassStack are always pushed/popped synchronously.
Why dont you keep a single vector of (StringRef, Optional<CFG>) pair to make this couple more obvious?


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:486
+
+    if (!PassPA.allAnalysesInSetPreserved(CFGAnalyses::ID()))
+      return;
----------------
This seems to be a more new-PM-idiomatic way of checking:
   PassPA.allAnalysesInSetPreserved<CFGAnalyses>()


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

https://reviews.llvm.org/D81558