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

[Cong Hou via llvm-commits]

On Fri, Oct 2, 2015 at 8:58 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

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

Thank you for the review! (Just came back from vacation)


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

OK.


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

Done.


>
> > +}
> > +
> > +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).
>

I have committed this change in another one.


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

OK.


>
> >  }
> >
> >  /// 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.
>

Done.


>
> > +
> > +  /// \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.
>

If we are replacing edge weights with probabilities, this function won't be
used anymore in the future. So I think it is OK to put it here and then
delete it in the future.


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

This is a better idea and an STL way. Thanks!


>
> >  };
> >
> > +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;
> > +}
> > +
> >  }
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151012/884d7cb8/attachment.html>