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

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
Comment at: include/llvm/IR/PassManager.h:98
+
+/// An analysis set representing the CFG of a function.
+///
----------------
This analysis set doesn't really "represent the CFG of a function".  It's like AllAnalysesOn above; it "represents analyses that only look at functions' control flow graphs (and therefore are preserved if the CFG is unchanged)".


================
Comment at: include/llvm/IR/PassManager.h:105
+/// between them. Mutating a branch instruction or basic block is enough to
+/// invalidate the CFG.
+class AnalysesOnFunctionCFG {
----------------
Can you clarify what you mean by "mutating a basic block"?  I would think that adding (say) an `add` instruction to a BB does not affect the CFG, but does "mutate" the BB.


================
Comment at: include/llvm/IR/PassManager.h:106
+/// invalidate the CFG.
+class AnalysesOnFunctionCFG {
+public:
----------------
You could call it `CFGAnalyses`?  If at some point we wanted to preserve the CFG plus call graph in every function in a module, we could call that "GlobalCFGAnalyses" and I don't think that would be particularly strange.

I don't think we need to keep with the `AllAnalysesOnFoo` idiom.  It makes sense for `AllAnalysesOn<IRUnitT>` (although if you wanted to drop the "All" I'd be ok with that), but that's mainly because of the position of the template parameter.


================
Comment at: include/llvm/IR/PassManager.h:170
+    return PA;
+  }
+
----------------
I actually think we should get rid of these two helpers and make the code more verbose, on the theory that there should be One Way to Do It.  Otherwise every time we add a new analysis set, we have to ask whether it belongs as a static function in here, and in any case our code is going to be inconsistent, with half using the one way and half using the other...  Also seeing the helper makes me ask the question, "is this different somehow than the trivial implementation I expected?"


================
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>());
----------------
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.


https://reviews.llvm.org/D28627