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

Fri Oct 2 20:58:48 PDT 2015

> On 2015-Oct-01, at 13:53, Cong Hou <congh at google.com> wrote:
> 
> Ping?
> 
> thanks,
> Cong
> 
> On Thu, Sep 24, 2015 at 11:35 AM, Cong Hou <congh at google.com> wrote:
> Hi Duncan
> 
> Could you please take a look at this patch? Thank you!
> 
> 
> thanks,
> Cong
> 
> On Tue, Sep 1, 2015 at 10:34 AM, Cong Hou <congh at google.com> wrote:
> Hi Duncan
> 
> Could you please take a look at this patch. It is refined again and now should look better.
> 
> Thanks!
> 
> Cong
> 
> ---------- Forwarded message ----------
> From: Cong Hou <congh at google.com>
> Date: Mon, Aug 31, 2015 at 11:14 AM
> Subject: Re: [PATCH] D10979: Update the branch weight metadata in JumpThreading pass.
> To: congh at google.com, chandlerc at gmail.com, davidxl at google.com, dexonsmith at apple.com
> Cc: dnovillo at google.com, llvm-commits at lists.llvm.org
> 
> 
> congh updated this revision to Diff 33602.
> congh added a comment.
> 
> Update the patch by adding the definition of MachineBranchProbabilityInfo::normalizeEdgeWeights().
> 
> 
> http://reviews.llvm.org/D10979
> 
> Files:
>   include/llvm/Analysis/BlockFrequencyInfo.h
>   include/llvm/Analysis/BlockFrequencyInfoImpl.h
>   include/llvm/CodeGen/MachineBranchProbabilityInfo.h
>   include/llvm/Support/BlockFrequency.h
>   lib/Analysis/BlockFrequencyInfo.cpp
>   lib/Analysis/BlockFrequencyInfoImpl.cpp
>   lib/Analysis/BranchProbabilityInfo.cpp
>   lib/Support/BlockFrequency.cpp
>   lib/Transforms/Scalar/JumpThreading.cpp
>   test/Transforms/JumpThreading/update-edge-weight.ll
> 
> 
> 
> 

This close, just a bunch of low-level stuff.  (Sorry for the long delay,
somehow I missed all the pings until now.)

Happy for you to fix up/move `normalizeEdgeWeights()` as a follow-up.
LGTM after you address the rest.

> Index: lib/Support/BlockFrequency.cpp
> ===================================================================
> --- lib/Support/BlockFrequency.cpp
> +++ lib/Support/BlockFrequency.cpp
> @@ -23,8 +23,7 @@
>    return *this;
>  }
>  
> -const BlockFrequency
> -BlockFrequency::operator*(const BranchProbability &Prob) const {
> +BlockFrequency BlockFrequency::operator*(const BranchProbability &Prob) const {

This appears to be NFC.  Please commit it separately.

>    BlockFrequency Freq(Frequency);
>    Freq *= Prob;
>    return Freq;
> @@ -52,11 +51,25 @@
>    return *this;
>  }
>  
> -const BlockFrequency
> -BlockFrequency::operator+(const BlockFrequency &Prob) const {
> -  BlockFrequency Freq(Frequency);
> -  Freq += Prob;
> -  return Freq;
> +BlockFrequency BlockFrequency::operator+(const BlockFrequency &Freq) const {
> +  BlockFrequency NewFreq(Frequency);
> +  NewFreq += Freq;
> +  return NewFreq;

This part of the patch seems to be NFC.  Please commit it separately to
simplify the patch.

> +}
> +
> +BlockFrequency &BlockFrequency::operator-=(const BlockFrequency &Freq) {
> +  // If underflow, set frequency to 0.
> +  if (Frequency < Freq.Frequency)
> +    Frequency = 0;
> +  else
> +    Frequency -= Freq.Frequency;
> +  return *this;
> +}
> +
> +BlockFrequency BlockFrequency::operator-(const BlockFrequency &Freq) const {
> +  BlockFrequency NewFreq(Frequency);
> +  NewFreq -= Freq;
> +  return NewFreq;
>  }

Please commit this new API separately and add good unit tests for it
(either `-=` or `-`, no need to test both).

>  
>  BlockFrequency &BlockFrequency::operator>>=(const unsigned count) {
> Index: lib/Analysis/BranchProbabilityInfo.cpp
> ===================================================================
> --- lib/Analysis/BranchProbabilityInfo.cpp
> +++ lib/Analysis/BranchProbabilityInfo.cpp
> @@ -604,7 +604,7 @@
>          Weight += MapI->second;
>        }
>      }
> -  return (!FoundWeight) ? DEFAULT_WEIGHT : Weight;
> +  return FoundWeight ? Weight : DEFAULT_WEIGHT;

This change seems completely unrelated (and non-functional).  Please
split things like this out into separate NFC commits.

>  }
>  
>  /// Set the edge weight for a given edge specified by PredBlock and an index
> Index: lib/Analysis/BlockFrequencyInfoImpl.cpp
> ===================================================================
> --- lib/Analysis/BlockFrequencyInfoImpl.cpp
> +++ lib/Analysis/BlockFrequencyInfoImpl.cpp
> @@ -530,6 +530,13 @@
>    return Freqs[Node.Index].Scaled;
>  }
>  
> +void BlockFrequencyInfoImplBase::setBlockFreq(const BlockNode &Node,
> +                                              uint64_t Freq) {
> +  assert(Node.isValid() && "Expected valid node");
> +  assert(Node.Index < Freqs.size() && "Expected legal index");
> +  Freqs[Node.Index].Integer = Freq;
> +}
> +
>  std::string
>  BlockFrequencyInfoImplBase::getBlockName(const BlockNode &Node) const {
>    return std::string();
> Index: lib/Analysis/BlockFrequencyInfo.cpp
> ===================================================================
> --- lib/Analysis/BlockFrequencyInfo.cpp
> +++ lib/Analysis/BlockFrequencyInfo.cpp
> @@ -129,6 +129,12 @@
>    return BFI ? BFI->getBlockFreq(BB) : 0;
>  }
>  
> +void BlockFrequencyInfo::setBlockFreq(const BasicBlock *BB,
> +                                      uint64_t Freq) {
> +  assert(BFI && "Expected analysis to be available");
> +  BFI->setBlockFreq(BB, Freq);
> +}
> +
>  /// Pop up a ghostview window with the current block frequency propagation
>  /// rendered using dot.
>  void BlockFrequencyInfo::view() const {
> Index: include/llvm/Support/BlockFrequency.h
> ===================================================================
> --- include/llvm/Support/BlockFrequency.h
> +++ include/llvm/Support/BlockFrequency.h
> @@ -38,16 +38,20 @@
>    /// \brief Multiplies with a branch probability. The computation will never
>    /// overflow.
>    BlockFrequency &operator*=(const BranchProbability &Prob);
> -  const BlockFrequency operator*(const BranchProbability &Prob) const;
> +  BlockFrequency operator*(const BranchProbability &Prob) const;
>  
>    /// \brief Divide by a non-zero branch probability using saturating
>    /// arithmetic.
>    BlockFrequency &operator/=(const BranchProbability &Prob);
>    BlockFrequency operator/(const BranchProbability &Prob) const;
>  
>    /// \brief Adds another block frequency using saturating arithmetic.
>    BlockFrequency &operator+=(const BlockFrequency &Freq);
> -  const BlockFrequency operator+(const BlockFrequency &Freq) const;
> +  BlockFrequency operator+(const BlockFrequency &Freq) const;

These changes seem to be NFC.  Please commit them separately.

> +
> +  /// \brief Subtract another block frequency using saturating arithmetic.
> +  BlockFrequency &operator-=(const BlockFrequency &Freq);
> +  BlockFrequency operator-(const BlockFrequency &Freq) const;
>  
>    /// \brief Shift block frequency to the right by count digits saturating to 1.
>    BlockFrequency &operator>>=(const unsigned count);
> Index: include/llvm/CodeGen/MachineBranchProbabilityInfo.h
> ===================================================================
> --- include/llvm/CodeGen/MachineBranchProbabilityInfo.h
> +++ include/llvm/CodeGen/MachineBranchProbabilityInfo.h
> @@ -83,8 +83,34 @@
>    raw_ostream &printEdgeProbability(raw_ostream &OS,
>                                      const MachineBasicBlock *Src,
>                                      const MachineBasicBlock *Dst) const;
> +
> +  // Normalize a list of weights by scaling them down so that the sum of them
> +  // doesn't exceed UINT32_MAX. Return the scale.
> +  template <class WeightList>
> +  static uint32_t normalizeEdgeWeights(WeightList &Weights);

I wonder if this should be a free function, maybe in `BlockFrequency.h`?
In particular, it seems like a good candidate for unit testing.

Although I'm not sure it's templated on the right thing; perhaps it
should take two iterators?

>  };
>  
> +template <class WeightList>
> +uint32_t
> +MachineBranchProbabilityInfo::normalizeEdgeWeights(WeightList &Weights) {
> +  assert(Weights.size() < UINT32_MAX && "Too many weights in the list!");
> +  // First we compute the sum with 64-bits of precision.
> +  uint64_t Sum = std::accumulate(Weights.begin(), Weights.end(), uint64_t(0));
> +
> +  // If the computed sum fits in 32-bits, we're done.
> +  if (Sum <= UINT32_MAX)
> +    return 1;
> +
> +  // Otherwise, compute the scale necessary to cause the weights to fit, and
> +  // re-sum with that scale applied.
> +  assert((Sum / UINT32_MAX) < UINT32_MAX &&
> +         "The sum of weights exceeds UINT32_MAX^2!");
> +  uint32_t Scale = (Sum / UINT32_MAX) + 1;
> +  for (auto &W : Weights)
> +    W /= Scale;
> +  return Scale;
> +}
> +
>  }
>  
>  