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

[Serguei Katkov via Phabricator via cfe-commits]

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

with two nits.




Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:422
+  // Register all the standard instrumentation callbacks. If \p FAM is nullptr
+  // then PreservedCFGChecker is not registeredenabled.
+  void registerCallbacks(PassInstrumentationCallbacks ,

yrouban wrote:
> fix registeredenabled
Not Done.



Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:1079
+const auto *F = any_cast(IR);
+FAM.getResult(*const_cast(F));
+  });

Add a comment that you caches the CFG before pass.


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

https://reviews.llvm.org/D91327

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Serguei Katkov via Phabricator via cfe-commits]

skatkov added a comment.

Consider landing tests update for "-verify-cfg-preserved=0" in a separate 
commit. This will significantly reduce the size of review.


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

https://reviews.llvm.org/D91327

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Serguei Katkov via Phabricator via cfe-commits]

skatkov added a comment.

First iteration: style comments.




Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:124
   if (BBGuards)
 for (auto  : *BBGuards) {
   if (BB.second.isPoisoned())

redundant {}
consider using any_of



Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:1017
 
+struct PreservedCFGCheckerAnalysis
+: public AnalysisInfoMixin {

Describe the idea of this analisys


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

https://reviews.llvm.org/D91327

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Serguei Katkov via Phabricator via cfe-commits]

skatkov added inline comments.



Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:769
+auto *F = any_cast(IR);
+if (auto *Result = FAM.getCachedResult(
+*const_cast(F)))

Result -> GraphBefore 



Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:771
+*const_cast(F)))
+  checkCFG(P, F->getName(), *Result, CFG(F, false /* TrackBBLifetime */));
 else

why do you check before pass runs?



Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:778
   [this](StringRef P, const PreservedAnalyses ) {
-auto Before = GraphStackBefore.pop_back_val();
-assert(Before.first == P &&
+assert(PassStack.pop_back_val() == P &&
"Before and After callbacks must correspond");

you should not PassStack.pop_back_val() for product build?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91327

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits