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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Thanks for all of the great review comments! Patch is updated, see responses below.



================
Comment at: include/llvm/IR/PassManager.h:98
+
+/// An analysis set representing the CFG of a function.
+///
----------------
jlebar wrote:
> 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)".
Tried to reword, lemme know if you like.


================
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 {
----------------
jlebar wrote:
> 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.
Sorry, I meant mutating the *set* of BBs. IE, adding or removing BBs. I also made the branch mutation comment much more accurate and precise (I hope).


================
Comment at: include/llvm/IR/PassManager.h:106
+/// invalidate the CFG.
+class AnalysesOnFunctionCFG {
+public:
----------------
jlebar wrote:
> 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.
Done, and thanks!


================
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:
> 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.
The reason just not calling these functions doesn't work is that we need to call these functions to handle dependencies between analyses.

I agree however that this is essentially the listing of dependencies and sets. If there is a more concise and simple way of writing the code however, I'm all for it.

I tried several variations to get a single method call on PreservedAnalyses, but they weren't actually easier to read.


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:3180-3181
+  auto PA = PreservedAnalyses::allOnFunctionCFG();
+  PA.preserve<AAManager>();
+  PA.preserve<GlobalsAA>();
   return PA;
----------------
davide wrote:
> The fact that we preserve now `GlobalsAA` and `AAManager` is an unrelated change, I guess? (can be committed separately).
Yeah, I just noticed when reading the code. I'll pull this into a separate commit as there is also a FIXME above that would be good to resolve at the same time.


================
Comment at: lib/Transforms/Scalar/GuardWidening.cpp:662-663
   bool Changed = GuardWideningImpl(DT, PDT, LI).run();
-  return Changed ? PreservedAnalyses::none() : PreservedAnalyses::all();
+  return Changed ? PreservedAnalyses::allOnFunctionCFG()
+                 : PreservedAnalyses::all();
 }
----------------
davide wrote:
> hmm, this doesn't seem to have a FIXME but it's supposed to preserve the CFG analyses, so good that you caught it (here and elsewhere).
I tried to audit all passes which have a setPreservesCFG for the legacy PM and a run function definition for the new PM, and add this to them.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2497
+  PA.preserveSet<AnalysesOnFunctionCFG>();
+  return std::move(PA);
 }
----------------
davide wrote:
> Why `std::move()` here?
Because i didn't remove it before mailing. Gone now.


================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:637
     return PreservedAnalyses::all();
+
   return getLoopPassPreservedAnalyses();
----------------
davide wrote:
> Unrelated?
I was just making all of these look the same. I'm happy to split it into a separate patch if you want, but didn't seem worthwhile..


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:72
     return PreservedAnalyses::all();
+
   return getLoopPassPreservedAnalyses();
----------------
davide wrote:
> Unrelated?
See above. Will follow whatever pattern you want.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:1133
     return PreservedAnalyses::all();
+
   return getLoopPassPreservedAnalyses();
----------------
davide wrote:
> Unrelated?
See above.


https://reviews.llvm.org/D28627