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

[David Li via llvm-commits]

davidxl added a comment.

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
3. weight meta data should be a direct translation of updated BPI.

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.

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

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


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1453
@@ +1452,3 @@
+  if (HasProfileData)
+    BFI->setBlockFreq(NewBB, BFI->getBlockFreq(PredBB).getFrequency());
+
----------------
Should the NewBB' freq = BFI->getBlockFreq(PredBB)*BPI->getEdgeProbability(PredBB, BB)?

Where is the BPI update for PredBB->NewBB?

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1571
@@ +1570,3 @@
+/// edge BB->SuccBB. This is done by scaling the weight of BB->SuccBB by 1 -
+/// Freq(PredBB->BB) / Freq(BB->SuccBB).
+void JumpThreading::UpdateBlockFreqAndEdgeWeight(BasicBlock *PredBB,
----------------
Freq(PredBB->NewBB) / Orig_Freq(BB->SuccBB)

which is basically Freq (NewBB)/Orig_Freq(BB->SuccBB)

================
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);
----------------
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).

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1591
@@ +1590,3 @@
+  // BB to zero.
+  uint64_t NewBBFreq =
+      (PredBB2BBFreq <= OrigBBFreq)
----------------
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.

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1595
@@ +1594,3 @@
+          : 0;
+  BFI->setBlockFreq(BB, NewBBFreq);
+
----------------
Here is the case where BB's frequency is updated, but the BPI for BB's successors are not updated. I think these updates needs to be wrapped into its own helper function independent of meta data weights update (mixing them makes the code hard to read and reason).

================
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));
----------------
This whole loop seems unnecessary.

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1621
@@ +1620,3 @@
+      }
+      Weights.push_back(W);
+    }
----------------
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.


http://reviews.llvm.org/D10979