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

[Cong Hou via llvm-commits]

congh marked 3 inline comments as done.
congh added a comment.

In http://reviews.llvm.org/D10979#234372, @davidxl wrote:

> After reading the code again, I think the change needs to be restructured. There are a couple of main points we should stick to:
>
> 1. When CFG is changed, the code range in which the in-memory profile data is in inconsistent state should be minimized -- ideally immediately after the CFG is changed -- so that the profile state before and after the CFG change is kosher.
> 2. BFI and BPI update need to be bundled together


BFI and BPI are updated in different ways and it is difficult to write a general function doing this. However, I have greatly reduced the size of the function that updates BFI and BPI in this case.

> 3. weight meta data should be a direct translation of updated BPI.


This is a good idea. I have used the edge frequencies to update edge weights. The only possible issue is that when the source BB's frequency is too low, we may suffer some precision loss. But I think it is OK because BB's frequency is low.

> I suggest reorganizing the change like this

> 

> 1. BFI and BPI update for threading should be in one place (one helper function). a) BBs involved:  PredBB, NewBB, BB, and SuccBB b) Edges involved: Pred->BB : removed Pred->NewBB: new NewBB->SuccBB: new BB->SuccBB: need edge freq and bp update.


This is now done in one function.

> 

> 

> 2. after threading profile update, the meta data update should be simple.


Right.

> There should be no worry for overflow as subtraction is involved. Due to this, the scaling code can be simple.


Right. As we now use edge frequency to calculate edge weights, the only think we should take care is that we need to normalize those frequencies to let them fit in 32-bit.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1596
@@ +1595,3 @@
+               "The weight on BB->SuccBB is not coherent with it in BPI");
+        if (BB2SuccBBFreq.getFrequency() > 0) {
+          DEBUG(dbgs() << "  Update the weight on the edge from "
----------------
congh wrote:
> davidxl wrote:
> > Why this condition?
> I forget to add a comment when I did this. But I think this condition can be removed now.
It was used to prevent the "Denominator cannot be 0!" error when BB2SuccBBFreq is used to build branch probability. I have also handled this case when scaling weights below.

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1453
@@ +1452,3 @@
+  if (HasProfileData)
+    BFI->setBlockFreq(NewBB, BFI->getBlockFreq(PredBB).getFrequency());
+
----------------
davidxl wrote:
> Should the NewBB' freq = BFI->getBlockFreq(PredBB)*BPI->getEdgeProbability(PredBB, BB)?
> 
> Where is the BPI update for PredBB->NewBB?
You are right. I have corrected this part.

The BPI update for PredBB->NewBB is unnecessary here as PredBB's old successor BB is replaced by NewBB and PredBB's weights list  in BPI can remain unchanged (in BPI each edge weight is mapped with a BB-to-success_index pair).

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1585
@@ +1584,3 @@
+  auto PredBB2BBFreq =
+      BFI->getBlockFreq(PredBB) * BPI->getEdgeProbability(PredBB, BB);
+  auto BB2SuccBBFreq = OrigBBFreq * BPI->getEdgeProbability(BB, SuccBB);
----------------
davidxl wrote:
> When NewBB is created, there should be no edge from PredBB to BB anymore. The code here assumes the BPI has not been updated which can be broken.
> 
> I think the right way is to use NewBB's frequency which is already computed -- the update interface either needs to pass in NewBB's freq or NewBB (clone of BB).
You are right. Here I should use NewBB to calculate the frequency of the edge from PredBB to BB.

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1591
@@ +1590,3 @@
+  // BB to zero.
+  uint64_t NewBBFreq =
+      (PredBB2BBFreq <= OrigBBFreq)
----------------
davidxl wrote:
> To avoid ambiguity of the name, it is better to change the name of the variable to BBNewFreq, otherwise it may be thought of as NewBB's frequency where NewBB is the cloned bb of 'BB'.
> 
> Similarly change OrigBBFreq to BBOrigFreq.
OK.

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1601
@@ +1600,3 @@
+    int SuccIndexToUpdate = -1;
+    for (unsigned MD_i = 1, MD_e = MD->getNumOperands(); MD_i < MD_e; ++MD_i) {
+      ConstantInt *CI = mdconst::extract<ConstantInt>(MD->getOperand(MD_i));
----------------
davidxl wrote:
> This whole loop seems unnecessary.
This is removed now as we are using edge frequencies to calculate edge weights.

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1621
@@ +1620,3 @@
+      }
+      Weights.push_back(W);
+    }
----------------
davidxl wrote:
> There does not seem to be a need to collect the old weights at all. As long as we have newly updated branch probabilty info (using the updated edge frequency), the new weights can be resynthezied.
This is done now.


http://reviews.llvm.org/D10979