[PATCH] D136827: [JT][CT] Preserve exisiting BPI/BFI during JumpThreading

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

Put some nits & potential bug comments. Do we know the actual CT impact of that?

@nikic could you please run your CT machinery to see if it gives something?



================
Comment at: llvm/include/llvm/Transforms/Scalar/JumpThreading.h:78
 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;
-  bool HasProfileData = false;
+  Function *F = nullptr;
+  FunctionAnalysisManager *FAM = nullptr;
----------------
Can be &Function (less changes below)


================
Comment at: llvm/include/llvm/Transforms/Scalar/JumpThreading.h:173
+
+  // Returns an existing instance of BPI/BFI if any, otherwise nullptr. By
+  // "existing" we mean either cached result provided by FunctionAnalysisManger
----------------
nit: three slashes before comments, makes sense to write them separately for both (yes, it's mostly copy-paste).


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:357
+#ifndef NDEBUG
+  DTU.flush();
+#if defined(EXPENSIVE_CHECKS)
----------------
Why this is under NDEBUG?


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:374
+  // TODO: Would be nice to verify BPI/BFI consistency similar to DominatorTree.
+//  PA.preserve<BranchProbabilityAnalysis>();
+//  PA.preserve<BlockFrequencyAnalysis>();
----------------
Bad formatting


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:423
+  HasProfile = llvm::any_of(*F, [&](BasicBlock &BB) {
+    return this->doesBlockHaveProfileData(&BB);
+  });
----------------
I think we should skil unreachable blocks here.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:1133
     ConstantFoldTerminator(BB, true, nullptr, DTU);
-    if (HasProfileData)
+    if (auto BPI = getBPI())
       BPI->eraseBlock(BB);
----------------
Is it guaranteed that BPI is created by this moment? How?

Should be getOrCreateBPI?


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:2738
   OldPredBranch->eraseFromParent();
-  if (HasProfileData)
+  if (auto BPI = getBPI())
     BPI->copyEdgeProbabilities(BB, PredBB);
----------------
`auto *`


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:3095
+  if (!BPI) {
+    assert(FAM && "Can't create BPI with out FunctionAnalysisManager");
+    BPI = FAM->getCachedResult<BranchProbabilityAnalysis>(*F);
----------------
nit: with out -> without (same below)


================
Comment at: llvm/test/Transforms/JumpThreading/threading_prof2.ll:42
 ;CHECK: ![[PROF1]] = !{!"branch_weights", i32 1073205, i32 2146410443}
-;CHECK: ![[PROF2]] = !{!"branch_weights", i32 2146410443, i32 1073205}
+;CHECK: ![[PROF2]] = !{!"branch_weights", i32 -2147483648, i32 0}
----------------
ebrevnov wrote:
> mkazantsev wrote:
> > This looks strange. What's the explanation here, is the old profile wrong?
> Yes, old behavior is wrong. Essentially it's just stale initial profile. Since initial probability to go from bb5 to bb7 is equal to calculated probability to go from bb to bb7 that means new probability to go from bb5 to bb7 is 0.
Got it, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136827