[PATCH] D69861: [JumpThreading] Factor out code to clone a basic block (NFC)

[Kazu Hirata via Phabricator via llvm-commits]

kazu marked 2 inline comments as done.
kazu added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:1976-1978
+/// Clone basic block BB except its terminator just for predecessor PredBB.
+/// Return the newly created basic block and the map from the variables in BB to
+/// the variables in the newly created basic block.
----------------
wmi wrote:
> It is a little weird for the interface to leave the insertion of BB terminator outside. Can we add SuccBB as another param, and include some processing after the call of CloneBasicBlock into this function?
I've decided to scale back and just copy instructions, not a basic block.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:1991-1996
+  // Set the block frequency of NewBB.
+  if (HasProfileData) {
+    auto NewBBFreq =
+        BFI->getBlockFreq(PredBB) * BPI->getEdgeProbability(PredBB, BB);
+    BFI->setBlockFreq(NewBB, NewBBFreq.getFrequency());
+  }
----------------
wmi wrote:
> The Block frequency update seems not precise enough. I feel it is more precise to use BB-->SuccBB branch probablity to compute the block frequency of NewBB. That can be a separated change from this refactoring, but it suggests we may want to include SuccBB as a param. 
I'm leaving the frequency calculation code where it is for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69861