[JumpThreading][NFC][CompileTime] Do not recompute BPI/BFI analyzes

[mkazantsev]

Summary

They can already be available, and even if not, DT/LI can be available. We should not recompute them.
Old PM is unchanged because it would require changing dependencies, and we don't care enough about it.

[mkazantsev]

My measurements are tad noisy, but they show ~10% JumpThreading CT saving on a big method.

[nikic]

llvm/lib/Transforms/Scalar/JumpThreading.cpp

327

You'd have to also register it with AnalysisUses and initialize a pass dependency. I'd suggest to leave the LegacyPM implementation alone.

[aeubanks]

llvm/lib/Transforms/Scalar/JumpThreading.cpp

357–358

can these two also go through the analysis manager?

[mkazantsev]

llvm/lib/Transforms/Scalar/JumpThreading.cpp

327

You're right.

[mkazantsev]

Addressed comments.

[nikic]

LG

[jyknight]

This change has caused our internal PGO+NewPM builds of clang to have become non-reproducible (that is: the binary is different every build, when built with a Clang that includes this change). The problem appears only when providing profile data. I am not sure why this change causes a problem -- perhaps some of the analysis data ought to have been invalidated by some other pass, but wasn't?

But, I've bisected to exactly this commit, so, I'm going to revert it while continuing to investigate.

[ebrevnov]

In D124439#3483470, @jyknight wrote:

This change has caused our internal PGO+NewPM builds of clang to have become non-reproducible (that is: the binary is different every build, when built with a Clang that includes this change). The problem appears only when providing profile data. I am not sure why this change causes a problem -- perhaps some of the analysis data ought to have been invalidated by some other pass, but wasn't?

But, I've bisected to exactly this commit, so, I'm going to revert it while continuing to investigate.

Hi! Any update on this?

Not sure if it has any impact on your case but there is a chance that JumpThreading updates profile metadata through updatePredecessorProfileMetadata and doesn't invalidate BPI/BFI analysis.

[ebrevnov]

In D124439#3731473, @ebrevnov wrote:

In D124439#3483470, @jyknight wrote:

This change has caused our internal PGO+NewPM builds of clang to have become non-reproducible (that is: the binary is different every build, when built with a Clang that includes this change). The problem appears only when providing profile data. I am not sure why this change causes a problem -- perhaps some of the analysis data ought to have been invalidated by some other pass, but wasn't?

But, I've bisected to exactly this commit, so, I'm going to revert it while continuing to investigate.

Hi! Any update on this?

Not sure if it has any impact on your case but there is a chance that JumpThreading updates profile metadata through updatePredecessorProfileMetadata and doesn't invalidate BPI/BFI analysis.

@jyknight, ping

llvm/lib/Transforms/Scalar/JumpThreading.cpp

bool JumpThreading::runOnFunction(Function &F) {
   auto TLI = &getAnalysis<TargetLibraryInfoWrapperPass>().getTLI(F);
   auto DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
   auto LVI = &getAnalysis<LazyValueInfoWrapperPass>().getLVI();
   auto AA = &getAnalysis<AAResultsWrapperPass>().getAAResults();
   DomTreeUpdater DTU(*DT, DomTreeUpdater::UpdateStrategy::Lazy);
   std::unique_ptr<BlockFrequencyInfo> BFI;
   std::unique_ptr<BranchProbabilityInfo> BPI;
   if (F.hasProfileData()) {
-    LoopInfo LI{DominatorTree(F)};
-    BPI.reset(new BranchProbabilityInfo(F, LI, TLI));
-    BFI.reset(new BlockFrequencyInfo(F, *BPI, LI));
+    auto LI = &getAnalysis<LoopInfoWrapperPass>().getLoopInfo();
+    BPI.reset(new BranchProbabilityInfo(F, *LI, TLI));
+    BFI.reset(new BlockFrequencyInfo(F, *BPI, *LI));
   }
 
   bool Changed = Impl.runImpl(F, TLI, TTI, LVI, AA, &DTU, F.hasProfileData(),
                               std::move(BFI), std::move(BPI));
   if (PrintLVIAfterJumpThreading) {
     dbgs() << "LVI for function '" << F.getName() << "':\n";
     LVI->printLVI(F, DTU.getDomTree(), dbgs());
   }
PreservedAnalyses JumpThreadingPass::run(Function &F,
   auto &DT = AM.getResult<DominatorTreeAnalysis>(F);
   auto &LVI = AM.getResult<LazyValueAnalysis>(F);
   auto &AA = AM.getResult<AAManager>(F);
   DomTreeUpdater DTU(DT, DomTreeUpdater::UpdateStrategy::Lazy);
 
   std::unique_ptr<BlockFrequencyInfo> BFI;
   std::unique_ptr<BranchProbabilityInfo> BPI;
   if (F.hasProfileData()) {
-    LoopInfo LI{DominatorTree(F)};
+    auto &LI = AM.getResult<LoopAnalysis>(F);
     BPI.reset(new BranchProbabilityInfo(F, LI, &TLI));
     BFI.reset(new BlockFrequencyInfo(F, *BPI, LI));
   }
 
   bool Changed = runImpl(F, &TLI, &TTI, &LVI, &AA, &DTU, F.hasProfileData(),
                          std::move(BFI), std::move(BPI));
 
   if (PrintLVIAfterJumpThreading) {
     dbgs() << "LVI for function '" << F.getName() << "':\n";
     LVI.printLVI(F, DTU.getDomTree(), dbgs());