[PATCH] D84959: [NewPM][LVI] Abandon LVI after CVP

Fri Jul 31 12:32:05 PDT 2020

aeubanks added inline comments.

================
Comment at: llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll:233
+; CHECK-O3-NEXT: Invalidating analysis: RegionInfoAnalysis
+; CHECK-O23SZ-NEXT: Clearing all analysis results for: foo
 ; CHECK-O-NEXT: Running pass: GlobalOptPass
----------------
aeubanks wrote:
> tejohnson wrote:
> > aeubanks wrote:
> > > nikic wrote:
> > > > The test diffs looks reasonable to me now, apart from this part. Is this expected?
> > > That is weird, I'll take a look.
> > I was just looking at this. The message "Clearing all analysis results for: <possibly invalidated loop>" comes from the callsite to AnalysisManager<IRUnitT, ExtraArgTs...>::clear from LoopAnalysisManagerFunctionProxy::Result::invalidate. Looking at the conditions for that call, I'm guessing that the following is no longer true:
> > PAC.preservedSet<AllAnalysesOn<Function>>() 
> > since the abandon() call will remove that ID from the preserved set.
> > 
> > So it is probably "expected" by the code change here. Not sure if there is a way to avoid it, i.e. is that code overly conservative.
> `DominatorTree::invalid()` checks `PAC.preserved() || PAC.preservedSet<AllAnalysesOn<Function>>() || PAC.preservedSet<CFGAnalyses>()`. Even though `PAC.preservedSet<AllAnalysesOn<Function>>()` will be false, `PAC.preserved()` should still be true.
Oh sorry, now I see what you mean, yes this is expected.
I don't follow the reasoning in the comments, but that's a different issue :)

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

https://reviews.llvm.org/D84959