[PATCH] D91327: [NewPM] Redesign of PreserveCFG Checker

[Yevgeny Rouban via Phabricator via cfe-commits]

yrouban added inline comments.


================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:134
 
 public:
   static cl::opt<bool> VerifyPreservedCFG;
----------------
kuhar wrote:
> not necessary anymore
there can bee a need to disabled/enable (e.g. for some tests or for debugging).


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:759
+    CFG::printDiff(dbgs(), GraphBefore, GraphAfter);
+    report_fatal_error(Twine("CFG unexpectedly changed by ", Pass));
+  };
----------------
kuhar wrote:
> I think this will print with `errs()`. Would it make sense to flush `dbgs()` ahead of printing with `errs()`?
I believe this comment is true for all callsites of  //report_fatal_error()//. If so it should be done inside.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:771
+            *const_cast<Function *>(F)))
+      checkCFG(P, F->getName(), *Result, CFG(F, false /* TrackBBLifetime */));
     else
----------------
skatkov wrote:
> why do you check before pass runs?
removed


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:778
       [this](StringRef P, const PreservedAnalyses &PassPA) {
-        auto Before = GraphStackBefore.pop_back_val();
-        assert(Before.first == P &&
+        assert(PassStack.pop_back_val() == P &&
                "Before and After callbacks must correspond");
----------------
skatkov wrote:
> you should not PassStack.pop_back_val() for product build?
PassStack is used only inside asserts


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:796
+    auto *F = any_cast<const Function *>(IR);
+    if (auto *GraphBefore = FAM.getCachedResult<PreservedCFGCheckerAnalysis>(
+            *const_cast<Function *>(F)))
----------------
to @skatkov
if //AfterPassCallback// is called before //AM.invalidate()// then we check CFG only if the condition (PassPA.allAnalysesInSetPreserved<CFGAnalyses>() || PassPA.allAnalysesInSetPreserved<AllAnalysesOn<Function>>()) holds.
if //AfterPassCallback// is called after //AM.invalidate()// then we check CFG only if the same condition holds and the PreservedCFGCheckerAnalysis is not invalidated. But it is invalidated according to the same condition.
All in all it does not matter if //AfterPassCallback// is called before or after //AM.invalidate()//.


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

https://reviews.llvm.org/D91327