[PATCH] D28627: [PM] Introduce an analysis set used to preserve all analyses over a function's CFG when that CFG is unchanged.

[Davide Italiano via Phabricator via llvm-commits]

davide added inline comments.


================
Comment at: lib/Analysis/BlockFrequencyInfo.cpp:135-143
+bool BlockFrequencyInfo::invalidate(Function &F, const PreservedAnalyses &PA,
+                                    FunctionAnalysisManager::Invalidator &) {
+  // Check whether the analysis, all analyses on functions, or the function's
+  // CFG have been preserved.
+  auto PAC = PA.getChecker<BlockFrequencyAnalysis>();
+  return !(PAC.preserved() || PAC.preservedSet<AllAnalysesOn<Function>>() ||
+           PAC.preservedSet<AnalysesOnFunctionCFG>());
----------------
jlebar wrote:
> davide wrote:
> > The class of `invalidate` functions share a decent amount of code, maybe we can factor it out somehow?
> The way I think of it, this here is how we list the things that preserve BlockFrequencyInfo.  "Factoring out" the code somehow would mean we changed the invalidator API somehow.
> 
> When I did an earlier review, I suggested not even calling invalidate() if we know a priori that the pass is preserved (I guess that would correspond to `PAC.preserved() || PAC.preservedSet<AllAnalysesOn<IRUnitT>>())`).  Chandler explained at the time why this wouldn't work.  I don't quite remember, but IIRC the problem isn't with analyses but with transformation passes, which want to do work inside `invalidate()`.  I think it's a decent simplification if we keep the analysis and transformation invalidate functions close together, even if it means we have to write a bit more boilerplate.
Fair enough.


https://reviews.llvm.org/D28627