<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Oct 2, 2015 at 8:58 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
> On 2015-Oct-01, at 13:53, Cong Hou <<a href="mailto:congh@google.com">congh@google.com</a>> wrote:<br>
><br>
> Ping?<br>
><br>
> thanks,<br>
> Cong<br>
><br>
> On Thu, Sep 24, 2015 at 11:35 AM, Cong Hou <<a href="mailto:congh@google.com">congh@google.com</a>> wrote:<br>
> Hi Duncan<br>
><br>
> Could you please take a look at this patch? Thank you!<br>
><br>
><br>
> thanks,<br>
> Cong<br>
><br>
> On Tue, Sep 1, 2015 at 10:34 AM, Cong Hou <<a href="mailto:congh@google.com">congh@google.com</a>> wrote:<br>
> Hi Duncan<br>
><br>
> Could you please take a look at this patch. It is refined again and now should look better.<br>
><br>
> Thanks!<br>
><br>
> Cong<br>
><br>
> ---------- Forwarded message ----------<br>
> From: Cong Hou <<a href="mailto:congh@google.com">congh@google.com</a>><br>
> Date: Mon, Aug 31, 2015 at 11:14 AM<br>
> Subject: Re: [PATCH] D10979: Update the branch weight metadata in JumpThreading pass.<br>
> To: <a href="mailto:congh@google.com">congh@google.com</a>, <a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>, <a href="mailto:davidxl@google.com">davidxl@google.com</a>, <a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a><br>
> Cc: <a href="mailto:dnovillo@google.com">dnovillo@google.com</a>, <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
><br>
><br>
> congh updated this revision to Diff 33602.<br>
> congh added a comment.<br>
><br>
> Update the patch by adding the definition of MachineBranchProbabilityInfo::normalizeEdgeWeights().<br>
><br>
><br>
> <a href="http://reviews.llvm.org/D10979" rel="noreferrer" target="_blank">http://reviews.llvm.org/D10979</a><br>
><br>
> Files:<br>
> include/llvm/Analysis/BlockFrequencyInfo.h<br>
> include/llvm/Analysis/BlockFrequencyInfoImpl.h<br>
> include/llvm/CodeGen/MachineBranchProbabilityInfo.h<br>
> include/llvm/Support/BlockFrequency.h<br>
> lib/Analysis/BlockFrequencyInfo.cpp<br>
> lib/Analysis/BlockFrequencyInfoImpl.cpp<br>
> lib/Analysis/BranchProbabilityInfo.cpp<br>
> lib/Support/BlockFrequency.cpp<br>
> lib/Transforms/Scalar/JumpThreading.cpp<br>
> test/Transforms/JumpThreading/update-edge-weight.ll<br>
><br>
><br>
><br>
><br>
<br>
</div></div>This close, just a bunch of low-level stuff. (Sorry for the long delay,<br>
somehow I missed all the pings until now.)<br></blockquote><div><br></div><div>Thank you for the review! (Just came back from vacation)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Happy for you to fix up/move `normalizeEdgeWeights()` as a follow-up.<br>
LGTM after you address the rest.<br>
<br>
> Index: lib/Support/BlockFrequency.cpp<br>
> ===================================================================<br>
> --- lib/Support/BlockFrequency.cpp<br>
> +++ lib/Support/BlockFrequency.cpp<br>
> @@ -23,8 +23,7 @@<br>
> return *this;<br>
> }<br>
><br>
> -const BlockFrequency<br>
> -BlockFrequency::operator*(const BranchProbability &Prob) const {<br>
> +BlockFrequency BlockFrequency::operator*(const BranchProbability &Prob) const {<br>
<br>
This appears to be NFC. Please commit it separately.<br></blockquote><div><br></div><div>OK.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> BlockFrequency Freq(Frequency);<br>
> Freq *= Prob;<br>
> return Freq;<br>
> @@ -52,11 +51,25 @@<br>
> return *this;<br>
> }<br>
><br>
> -const BlockFrequency<br>
> -BlockFrequency::operator+(const BlockFrequency &Prob) const {<br>
> - BlockFrequency Freq(Frequency);<br>
> - Freq += Prob;<br>
> - return Freq;<br>
> +BlockFrequency BlockFrequency::operator+(const BlockFrequency &Freq) const {<br>
> + BlockFrequency NewFreq(Frequency);<br>
> + NewFreq += Freq;<br>
> + return NewFreq;<br>
<br>
This part of the patch seems to be NFC. Please commit it separately to<br>
simplify the patch.<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +}<br>
> +<br>
> +BlockFrequency &BlockFrequency::operator-=(const BlockFrequency &Freq) {<br>
> + // If underflow, set frequency to 0.<br>
> + if (Frequency < Freq.Frequency)<br>
> + Frequency = 0;<br>
> + else<br>
> + Frequency -= Freq.Frequency;<br>
> + return *this;<br>
> +}<br>
> +<br>
<span class="">> +BlockFrequency BlockFrequency::operator-(const BlockFrequency &Freq) const {<br>
> + BlockFrequency NewFreq(Frequency);<br>
> + NewFreq -= Freq;<br>
</span>> + return NewFreq;<br>
> }<br>
<br>
Please commit this new API separately and add good unit tests for it<br>
(either `-=` or `-`, no need to test both).<br></blockquote><div><br></div><div>I have committed this change in another one.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
><br>
> BlockFrequency &BlockFrequency::operator>>=(const unsigned count) {<br>
> Index: lib/Analysis/BranchProbabilityInfo.cpp<br>
> ===================================================================<br>
> --- lib/Analysis/BranchProbabilityInfo.cpp<br>
> +++ lib/Analysis/BranchProbabilityInfo.cpp<br>
> @@ -604,7 +604,7 @@<br>
> Weight += MapI->second;<br>
> }<br>
> }<br>
> - return (!FoundWeight) ? DEFAULT_WEIGHT : Weight;<br>
> + return FoundWeight ? Weight : DEFAULT_WEIGHT;<br>
<br>
This change seems completely unrelated (and non-functional). Please<br>
split things like this out into separate NFC commits.<br></blockquote><div><br></div><div>OK.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> }<br>
><br>
> /// Set the edge weight for a given edge specified by PredBlock and an index<br>
> Index: lib/Analysis/BlockFrequencyInfoImpl.cpp<br>
> ===================================================================<br>
> --- lib/Analysis/BlockFrequencyInfoImpl.cpp<br>
> +++ lib/Analysis/BlockFrequencyInfoImpl.cpp<br>
> @@ -530,6 +530,13 @@<br>
> return Freqs[Node.Index].Scaled;<br>
> }<br>
><br>
> +void BlockFrequencyInfoImplBase::setBlockFreq(const BlockNode &Node,<br>
> + uint64_t Freq) {<br>
> + assert(Node.isValid() && "Expected valid node");<br>
> + assert(Node.Index < Freqs.size() && "Expected legal index");<br>
> + Freqs[Node.Index].Integer = Freq;<br>
> +}<br>
> +<br>
> std::string<br>
> BlockFrequencyInfoImplBase::getBlockName(const BlockNode &Node) const {<br>
> return std::string();<br>
> Index: lib/Analysis/BlockFrequencyInfo.cpp<br>
> ===================================================================<br>
> --- lib/Analysis/BlockFrequencyInfo.cpp<br>
> +++ lib/Analysis/BlockFrequencyInfo.cpp<br>
> @@ -129,6 +129,12 @@<br>
> return BFI ? BFI->getBlockFreq(BB) : 0;<br>
> }<br>
><br>
> +void BlockFrequencyInfo::setBlockFreq(const BasicBlock *BB,<br>
> + uint64_t Freq) {<br>
> + assert(BFI && "Expected analysis to be available");<br>
> + BFI->setBlockFreq(BB, Freq);<br>
> +}<br>
> +<br>
> /// Pop up a ghostview window with the current block frequency propagation<br>
> /// rendered using dot.<br>
> void BlockFrequencyInfo::view() const {<br>
> Index: include/llvm/Support/BlockFrequency.h<br>
> ===================================================================<br>
> --- include/llvm/Support/BlockFrequency.h<br>
> +++ include/llvm/Support/BlockFrequency.h<br>
> @@ -38,16 +38,20 @@<br>
> /// \brief Multiplies with a branch probability. The computation will never<br>
> /// overflow.<br>
> BlockFrequency &operator*=(const BranchProbability &Prob);<br>
> - const BlockFrequency operator*(const BranchProbability &Prob) const;<br>
> + BlockFrequency operator*(const BranchProbability &Prob) const;<br>
><br>
> /// \brief Divide by a non-zero branch probability using saturating<br>
> /// arithmetic.<br>
> BlockFrequency &operator/=(const BranchProbability &Prob);<br>
> BlockFrequency operator/(const BranchProbability &Prob) const;<br>
><br>
> /// \brief Adds another block frequency using saturating arithmetic.<br>
> BlockFrequency &operator+=(const BlockFrequency &Freq);<br>
> - const BlockFrequency operator+(const BlockFrequency &Freq) const;<br>
> + BlockFrequency operator+(const BlockFrequency &Freq) const;<br>
<br>
These changes seem to be NFC. Please commit them separately.<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +<br>
> + /// \brief Subtract another block frequency using saturating arithmetic.<br>
> + BlockFrequency &operator-=(const BlockFrequency &Freq);<br>
> + BlockFrequency operator-(const BlockFrequency &Freq) const;<br>
><br>
> /// \brief Shift block frequency to the right by count digits saturating to 1.<br>
> BlockFrequency &operator>>=(const unsigned count);<br>
> Index: include/llvm/CodeGen/MachineBranchProbabilityInfo.h<br>
> ===================================================================<br>
> --- include/llvm/CodeGen/MachineBranchProbabilityInfo.h<br>
> +++ include/llvm/CodeGen/MachineBranchProbabilityInfo.h<br>
> @@ -83,8 +83,34 @@<br>
> raw_ostream &printEdgeProbability(raw_ostream &OS,<br>
> const MachineBasicBlock *Src,<br>
> const MachineBasicBlock *Dst) const;<br>
> +<br>
> + // Normalize a list of weights by scaling them down so that the sum of them<br>
> + // doesn't exceed UINT32_MAX. Return the scale.<br>
> + template <class WeightList><br>
> + static uint32_t normalizeEdgeWeights(WeightList &Weights);<br>
<br>
I wonder if this should be a free function, maybe in `BlockFrequency.h`?<br>
In particular, it seems like a good candidate for unit testing.<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Although I'm not sure it's templated on the right thing; perhaps it<br>
should take two iterators?<br></blockquote><div><br></div><div>This is a better idea and an STL way. Thanks!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> };<br>
><br>
> +template <class WeightList><br>
> +uint32_t<br>
> +MachineBranchProbabilityInfo::normalizeEdgeWeights(WeightList &Weights) {<br>
> + assert(Weights.size() < UINT32_MAX && "Too many weights in the list!");<br>
> + // First we compute the sum with 64-bits of precision.<br>
> + uint64_t Sum = std::accumulate(Weights.begin(), Weights.end(), uint64_t(0));<br>
> +<br>
> + // If the computed sum fits in 32-bits, we're done.<br>
> + if (Sum <= UINT32_MAX)<br>
> + return 1;<br>
> +<br>
> + // Otherwise, compute the scale necessary to cause the weights to fit, and<br>
> + // re-sum with that scale applied.<br>
> + assert((Sum / UINT32_MAX) < UINT32_MAX &&<br>
> + "The sum of weights exceeds UINT32_MAX^2!");<br>
> + uint32_t Scale = (Sum / UINT32_MAX) + 1;<br>
> + for (auto &W : Weights)<br>
> + W /= Scale;<br>
> + return Scale;<br>
> +}<br>
> +<br>
> }<br>
><br>
><br>
<br>
</blockquote></div><br></div></div>