[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

353–355

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/include/llvm/Transforms/Scalar/JumpThreading.h

 /// In this case, the unconditional branch at the end of the first if can be
 /// revectored to the false side of the second if.
 class JumpThreadingPass : public PassInfoMixin<JumpThreadingPass> {
   TargetLibraryInfo *TLI;
   TargetTransformInfo *TTI;
   LazyValueInfo *LVI;
   AAResults *AA;
   DomTreeUpdater *DTU;
-  std::unique_ptr<BlockFrequencyInfo> BFI;
-  std::unique_ptr<BranchProbabilityInfo> BPI;
+  BlockFrequencyInfo *BFI;
+  BranchProbabilityInfo *BPI;
   bool HasProfileData = false;
   bool HasGuards = false;
 #ifndef LLVM_ENABLE_ABI_BREAKING_CHECKS
   SmallPtrSet<const BasicBlock *, 16> LoopHeaders;
 #else
   SmallSet<AssertingVH<const BasicBlock>, 16> LoopHeaders;
 #endif
 
   unsigned BBDupThreshold;
   unsigned DefaultBBDupThreshold;
   bool InsertFreezeWhenUnfoldingSelect;
 
 public:
   JumpThreadingPass(bool InsertFreezeWhenUnfoldingSelect = false, int T = -1);
 
   // Glue for old PM.
   bool runImpl(Function &F, TargetLibraryInfo *TLI, TargetTransformInfo *TTI,
                LazyValueInfo *LVI, AAResults *AA, DomTreeUpdater *DTU,
-               bool HasProfileData, std::unique_ptr<BlockFrequencyInfo> BFI,
-               std::unique_ptr<BranchProbabilityInfo> BPI);
+               bool HasProfileData, BlockFrequencyInfo *BFI,
+               BranchProbabilityInfo *BPI);
 
   PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
 
-  void releaseMemory() {
-    BFI.reset();
-    BPI.reset();
-  }
-
   void findLoopHeaders(Function &F);
   bool processBlock(BasicBlock *BB);
   bool maybeMergeBasicBlockIntoOnlyPred(BasicBlock *BB);
   void updateSSA(BasicBlock *BB, BasicBlock *NewBB,
                  DenseMap<Instruction *, Value *> &ValueMapping);
   DenseMap<Instruction *, Value *> cloneInstructions(BasicBlock::iterator BI,
                                                      BasicBlock::iterator BE,
                                                      BasicBlock *NewBB,

llvm/lib/Transforms/Scalar/JumpThreading.cpp

void getAnalysisUsage(AnalysisUsage &AU) const override {
       AU.addPreserved<DominatorTreeWrapperPass>();
       AU.addRequired<AAResultsWrapperPass>();
       AU.addRequired<LazyValueInfoWrapperPass>();
       AU.addPreserved<LazyValueInfoWrapperPass>();
       AU.addPreserved<GlobalsAAWrapperPass>();
       AU.addRequired<TargetLibraryInfoWrapperPass>();
       AU.addRequired<TargetTransformInfoWrapperPass>();
     }
-
-    void releaseMemory() override { Impl.releaseMemory(); }
   };
 
 } // end anonymous namespace
 
 char JumpThreading::ID = 0;
 
 INITIALIZE_PASS_BEGIN(JumpThreading, "jump-threading",
                 "Jump Threading", false, false)
bool JumpThreading::runOnFunction(Function &F) {
   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));
   }
 
   bool Changed = Impl.runImpl(F, TLI, TTI, LVI, AA, &DTU, F.hasProfileData(),
-                              std::move(BFI), std::move(BPI));
+                              BFI.get(), BPI.get());
   if (PrintLVIAfterJumpThreading) {
     dbgs() << "LVI for function '" << F.getName() << "':\n";
     LVI->printLVI(F, DTU.getDomTree(), dbgs());
   }
   return Changed;
 }
 
 PreservedAnalyses JumpThreadingPass::run(Function &F,
                                          FunctionAnalysisManager &AM) {
   auto &TTI = AM.getResult<TargetIRAnalysis>(F);
   // Jump Threading has no sense for the targets with divergent CF
   if (TTI.hasBranchDivergence())
     return PreservedAnalyses::all();
   auto &TLI = AM.getResult<TargetLibraryAnalysis>(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;
+  BlockFrequencyInfo *BFI = nullptr;
+  BranchProbabilityInfo *BPI = nullptr;
   if (F.hasProfileData()) {
-    LoopInfo LI{DominatorTree(F)};
-    BPI.reset(new BranchProbabilityInfo(F, LI, &TLI));
-    BFI.reset(new BlockFrequencyInfo(F, *BPI, LI));
+    BFI = &AM.getResult<BlockFrequencyAnalysis>(F);
+    BPI = &AM.getResult<BranchProbabilityAnalysis>(F);
   }
 
-  bool Changed = runImpl(F, &TLI, &TTI, &LVI, &AA, &DTU, F.hasProfileData(),
-                         std::move(BFI), std::move(BPI));
+  bool Changed =
+      runImpl(F, &TLI, &TTI, &LVI, &AA, &DTU, F.hasProfileData(), BFI, BPI);
 
   if (PrintLVIAfterJumpThreading) {
     dbgs() << "LVI for function '" << F.getName() << "':\n";
     LVI.printLVI(F, DTU.getDomTree(), dbgs());
   }
 
   if (!Changed)
     return PreservedAnalyses::all();
   PreservedAnalyses PA;
   PA.preserve<DominatorTreeAnalysis>();
   PA.preserve<LazyValueAnalysis>();
   return PA;
 }
 
 bool JumpThreadingPass::runImpl(Function &F, TargetLibraryInfo *TLI_,
                                 TargetTransformInfo *TTI_, LazyValueInfo *LVI_,
                                 AliasAnalysis *AA_, DomTreeUpdater *DTU_,
-                                bool HasProfileData_,
-                                std::unique_ptr<BlockFrequencyInfo> BFI_,
-                                std::unique_ptr<BranchProbabilityInfo> BPI_) {
+                                bool HasProfileData_, BlockFrequencyInfo *BFI_,
+                                BranchProbabilityInfo *BPI_) {
   LLVM_DEBUG(dbgs() << "Jump threading on function '" << F.getName() << "'\n");
   TLI = TLI_;
   TTI = TTI_;
   LVI = LVI_;
   AA = AA_;
   DTU = DTU_;
-  BFI.reset();
-  BPI.reset();
+  BFI = BFI_;
+  BPI = BPI_;
   // When profile data is available, we need to update edge weights after
   // successful jump threading, which requires both BPI and BFI being available.
   HasProfileData = HasProfileData_;
   auto *GuardDecl = F.getParent()->getFunction(
       Intrinsic::getName(Intrinsic::experimental_guard));
   HasGuards = GuardDecl && !GuardDecl->use_empty();
   if (HasProfileData) {
-    BPI = std::move(BPI_);
-    BFI = std::move(BFI_);
+    assert(BFI && "BFI not provided?");
+    assert(BPI && "BPI not provided?");
+  } else {
+    assert(!BFI && "BFI should not be provided?");
+    assert(!BPI && "BPI should not be provided?");
   }
 
   // Reduce the number of instructions duplicated when optimizing strictly for
   // size.
   if (BBDuplicateThreshold.getNumOccurrences())
     BBDupThreshold = BBDuplicateThreshold;
   else if (F.hasFnAttribute(Attribute::MinSize))
     BBDupThreshold = 3;