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

[Cong Hou]

On Thu, Jul 16, 2015 at 5:33 PM, David <davidxl at google.com> wrote:
> 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'

OK.

>
> ================
> 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?

I will add assert on BFI/BPI 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.

OK.

>
> ================
> 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?

Yes, it's here: https://llvm.org/bugs/show_bug.cgi?id=23758

>
> ================
> 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

Yes, this is a good idea.

>
> ================
> 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

Right.

>
> ================
> 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?

This is the same issue as the one above.

>
> ================
> 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
>

Here I am actually using the old BB->SuccBB Frequency as

BPI->setEdgeWeight({BB, SuccBB}, W);

is called after the weight on BB->SuccBB is updated. So the
probability is still the old one.

>
> http://reviews.llvm.org/D10979
>
>
>