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

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:325
+  bool Changed =
+      Impl.runImpl(F, nullptr, TLI, TTI, LVI, AA, &DTU, BFI.get(), BPI.get());
   if (PrintLVIAfterJumpThreading) {
----------------
mkazantsev wrote:
> I guess old PM support is in decline, not sure if we want this here...
We don't change legacy behavior in any change. This is just a mechanical change to keep code compilable.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:2506
+  auto BFI = getBFI();
+  auto BPI = getBPI();
+  assert(
----------------
mkazantsev wrote:
> nit: `auto *`
Ok, will fix


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:3100
+    // BranchProbabilityAnalysis depends on DT. Make sure DT is consistent.
+    DTU->flush();
+    BPI = &FAM->getResult<BranchProbabilityAnalysis>(*F);
----------------
mkazantsev wrote:
> Would be nice to verify DT here in assertion mode.
I think it makes sense to perform "fast" verification here and "full" at the end of the pass under EXPENSIVE_CHECKS guard. WDYT?


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:3106
+
+BlockFrequencyInfo *JumpThreadingPass::getOrCreateBFI() {
+  if (HasProfile && (!BFI || *BFI == nullptr)) {
----------------
mkazantsev wrote:
> They are only called together; do we really need two separate functions?
There are number of place where we request BPI only...


================
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}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136827