[PATCH] D10979: Update the branch weight metadata in JumpThreading pass.

[Cong Hou via llvm-commits]

congh added inline comments.

================
Comment at: lib/Support/BlockFrequency.cpp:70
@@ +69,3 @@
+BlockFrequency BlockFrequency::operator-(const BlockFrequency &Freq) const {
+  BlockFrequency NewFreq(Frequency);
+  NewFreq -= Freq;
----------------
davidxl wrote:
> No underflow check here?
-= below is using the new operator defined above, so underflow is checked.

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1591
@@ +1590,3 @@
+  auto BBOrigFreq = BFI->getBlockFreq(BB);
+  auto PredBB2BBFreq = BFI->getBlockFreq(NewBB);
+  auto BB2SuccBBFreq = BBOrigFreq * BPI->getEdgeProbability(BB, SuccBB);
----------------
davidxl wrote:
> It is better to call PredBB2BBFreq NewBBFreq (with comment: frequency of the newly split BB)
OK.

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1601
@@ +1600,3 @@
+    auto SuccFreq = (*I == SuccBB)
+                        ? BB2SuccBBFreq - PredBB2BBFreq
+                        : BBOrigFreq * BPI->getEdgeProbability(BB, *I);
----------------
davidxl wrote:
> operator - does not have underflow check (see comment above).
But -= has underflow check which is used in operator -.

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1607
@@ +1606,3 @@
+  // Normalize edge weights in Weights64 so that the sum of them can fit in
+  MachineBranchProbabilityInfo::normalizeEdgeWeights(BBSuccFreq);
+
----------------
davidxl wrote:
> Is this interface available in  trunk?
Not yet :( Still waiting for the review of that patch.

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1616
@@ +1615,3 @@
+    BPI->setEdgeWeight(BB, I, Weights[I]);
+
+  if (Weights.size() >= 2) {
----------------
davidxl wrote:
> BPI for PredBB also needs to be updated for completeness -- as its successor BB is now NewBB
The old PredBB's successor BB is replaced by NewBB in the same position, so BPI doesn't have to be updated. (In BPI, the successor weights are stores in a map with the index of the successor as the key. The NewBB is set as a successor using the following code:

    if (PredTerm->getSuccessor(i) == BB) {
      BB->removePredecessor(PredBB, true);
      PredTerm->setSuccessor(i, NewBB);
    }

So the position of this new successor is not changed.)


http://reviews.llvm.org/D10979