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

[David]

davidxl added inline comments.

================
Comment at: include/llvm/Analysis/BlockFrequencyInfoImpl.h:972
@@ +971,3 @@
+template <class BT>
+void BlockFrequencyInfoImpl<BT>::setBlockFreq(const BlockT *BB, uint64_t Freq) {
+  if (Nodes.count(BB))
----------------
It might be slightly better to split the interfaces into two: one is set and the other is update.

================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:622
@@ +621,3 @@
+/// Set the edge weight for a given edge specified by Src and Dst.
+void BranchProbabilityInfo::setEdgeWeight(
+    const std::pair<const BasicBlock *, const BasicBlock *> &Edge,
----------------
It is probably better to call this one 'updateEdgeWeight'

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:150
@@ +149,3 @@
+                                const char *Suffix);
+    void UpdateEdgeWeight(BasicBlock *PredBB, BasicBlock *BB,
+                          BasicBlock *SuccBB);
----------------
This function certainly does more than just edge weight update -- a different name may be better.

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1560
@@ +1559,3 @@
+                                     BasicBlock *SuccBB) {
+  if (!HasProfileData)
+    return;
----------------
assert here?

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1568
@@ +1567,3 @@
+  // Theoretically PredBBFreq should not be greater than BBFreq. However, in
+  // some cases this may happen when the frequencies are inccorectly
+  // distributed to CFG nodes. In this case we just set the new frequency of
----------------
fix typo.

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1569
@@ +1568,3 @@
+  // some cases this may happen when the frequencies are inccorectly
+  // distributed to CFG nodes. In this case we just set the new frequency of
+  // BB to zero.
----------------
A side question: do we have a bug tracking that data insanity issue?

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1583
@@ +1582,3 @@
+      if (TI->getSuccessor(MD_i - 1) == SuccBB) {
+        auto BBFreq =
+            BFI->getBlockFreq(BB) * BPI->getEdgeProbability(BB, SuccBB);
----------------
Should probably assert here that BPI->getEdgeWeight(..) == W

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1583
@@ +1582,3 @@
+      if (TI->getSuccessor(MD_i - 1) == SuccBB) {
+        auto BBFreq =
+            BFI->getBlockFreq(BB) * BPI->getEdgeProbability(BB, SuccBB);
----------------
davidxl wrote:
> Should probably assert here that BPI->getEdgeWeight(..) == W
This is not really BBFreq, it should be BB2Succ Edge frequency

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1588
@@ +1587,3 @@
+                       << BB->getName() << " to " << SuccBB->getName() << "\n");
+          if (PredBBFreq <= BBFreq)
+            // Use the multiplication between BlockFrequency and
----------------
Another insanity to track?

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1591
@@ +1590,3 @@
+            // BranchProbability to avoid overflow.
+            W -= (BlockFrequency(W) *
+                  BranchProbability(PredBBFreq.getFrequency(),
----------------
I don't think this one is correct. You are using the New BB->Succ Frequency in the update which is wrong.

The invariant is :

PredBBFreq + NewBB2SuccBBEdgeFreq = OldBB2SuccBBEdgeFreq

and the update is:

NewWeight = (NewBB2SuccBBEdgeFreq/OldBB2EdgeFreq)* OldWeight

combine them we have

NewWeight = (1 - PredBBFreq/OldBB2EdgeFreq)*OldWeight


http://reviews.llvm.org/D10979