[llvm] r223500 - BFI: Saturate when combining edges to a successor

[Duncan P. N. Exon Smith]

+atrick,chandlerc

> On 2014 Dec 8, at 09:29, Chris Lattner <sabre at nondot.org> wrote:
> 
> 
>> On Dec 8, 2014, at 8:52 AM, Tom Stellard <tom at stellard.net> wrote:
>> 
>> On Fri, Dec 05, 2014 at 06:36:41PM -0800, Duncan P. N. Exon Smith wrote:
>>> Tom,
>>> 
>>> Is this okay for 3.5?
>>> 
>> 
>> cc'ing Chris who is the default code owner.
> 
> LGTM, though I’m not the best to review this.   Maybe we should get a profiling/coverage code owner?

I've CC'ed Andy and Chandler, who reviewed my work when I rewrote this
analysis in the spring.

> 
> -Chris
>> 
>> -Tom
>> 
>>>> On 2014 Dec 5, at 11:13, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>>> 
>>>> Author: dexonsmith
>>>> Date: Fri Dec  5 13:13:42 2014
>>>> New Revision: 223500
>>>> 
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=223500&view=rev
>>>> Log:
>>>> BFI: Saturate when combining edges to a successor
>>>> 
>>>> When a loop gets bundled up, its outgoing edges are quite large, and can
>>>> just barely overflow 64-bits.  If one successor has multiple incoming
>>>> edges -- and that successor is getting all the incoming mass --
>>>> combining just its edges can overflow.  Handle that by saturating rather
>>>> than asserting.
>>>> 
>>>> This fixes PR21622.
>>>> 
>>>> Added:
>>>>  llvm/trunk/test/Analysis/BlockFrequencyInfo/extremely-likely-loop-successor.ll
>>>> Modified:
>>>>  llvm/trunk/lib/Analysis/BlockFrequencyInfoImpl.cpp
>>>> 
>>>> Modified: llvm/trunk/lib/Analysis/BlockFrequencyInfoImpl.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/BlockFrequencyInfoImpl.cpp?rev=223500&r1=223499&r2=223500&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/Analysis/BlockFrequencyInfoImpl.cpp (original)
>>>> +++ llvm/trunk/lib/Analysis/BlockFrequencyInfoImpl.cpp Fri Dec  5 13:13:42 2014
>>>> @@ -14,6 +14,7 @@
>>>> #include "llvm/Analysis/BlockFrequencyInfoImpl.h"
>>>> #include "llvm/ADT/SCCIterator.h"
>>>> #include "llvm/Support/raw_ostream.h"
>>>> +#include <numeric>
>>>> 
>>>> using namespace llvm;
>>>> using namespace llvm::bfi_detail;
>>>> @@ -122,8 +123,12 @@ static void combineWeight(Weight &W, con
>>>> }
>>>> assert(W.Type == OtherW.Type);
>>>> assert(W.TargetNode == OtherW.TargetNode);
>>>> -  assert(W.Amount < W.Amount + OtherW.Amount && "Unexpected overflow");
>>>> -  W.Amount += OtherW.Amount;
>>>> +  assert(OtherW.Amount && "Expected non-zero weight");
>>>> +  if (W.Amount > W.Amount + OtherW.Amount)
>>>> +    // Saturate on overflow.
>>>> +    W.Amount = UINT64_MAX;
>>>> +  else
>>>> +    W.Amount += OtherW.Amount;
>>>> }
>>>> static void combineWeightsBySorting(WeightList &Weights) {
>>>> // Sort so edges to the same node are adjacent.
>>>> @@ -206,11 +211,19 @@ void Distribution::normalize() {
>>>>   Shift = 33 - countLeadingZeros(Total);
>>>> 
>>>> // Early exit if nothing needs to be scaled.
>>>> -  if (!Shift)
>>>> +  if (!Shift) {
>>>> +    // If we didn't overflow then combineWeights() shouldn't have changed the
>>>> +    // sum of the weights, but let's double-check.
>>>> +    assert(Total == std::accumulate(Weights.begin(), Weights.end(), UINT64_C(0),
>>>> +                                    [](uint64_t Sum, const Weight &W) {
>>>> +                      return Sum + W.Amount;
>>>> +                    }) &&
>>>> +           "Expected total to be correct");
>>>>   return;
>>>> +  }
>>>> 
>>>> // Recompute the total through accumulation (rather than shifting it) so that
>>>> -  // it's accurate after shifting.
>>>> +  // it's accurate after shifting and any changes combineWeights() made above.
>>>> Total = 0;
>>>> 
>>>> // Sum the weights to each node and shift right if necessary.
>>>> 
>>>> Added: llvm/trunk/test/Analysis/BlockFrequencyInfo/extremely-likely-loop-successor.ll
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/BlockFrequencyInfo/extremely-likely-loop-successor.ll?rev=223500&view=auto
>>>> ==============================================================================
>>>> --- llvm/trunk/test/Analysis/BlockFrequencyInfo/extremely-likely-loop-successor.ll (added)
>>>> +++ llvm/trunk/test/Analysis/BlockFrequencyInfo/extremely-likely-loop-successor.ll Fri Dec  5 13:13:42 2014
>>>> @@ -0,0 +1,40 @@
>>>> +; RUN: opt < %s -analyze -block-freq | FileCheck %s
>>>> +
>>>> +; PR21622: Check for a crasher when the sum of exits to the same successor of a
>>>> +; loop overflows.
>>>> +
>>>> +; CHECK-LABEL: Printing analysis {{.*}} for function 'extremely_likely_loop_successor':
>>>> +; CHECK-NEXT: block-frequency-info: extremely_likely_loop_successor
>>>> +define void @extremely_likely_loop_successor() {
>>>> +; CHECK-NEXT: entry: float = 1.0, int = [[ENTRY:[0-9]+]]
>>>> +entry:
>>>> +  br label %loop
>>>> +
>>>> +; CHECK-NEXT: loop: float = 1.0,
>>>> +loop:
>>>> +  %exit.1.cond = call i1 @foo()
>>>> +  br i1 %exit.1.cond, label %exit, label %loop.2, !prof !0
>>>> +
>>>> +; CHECK-NEXT: loop.2: float = 0.0000000
>>>> +loop.2:
>>>> +  %exit.2.cond = call i1 @foo()
>>>> +  br i1 %exit.2.cond, label %exit, label %loop.3, !prof !0
>>>> +
>>>> +; CHECK-NEXT: loop.3: float = 0.0000000
>>>> +loop.3:
>>>> +  %exit.3.cond = call i1 @foo()
>>>> +  br i1 %exit.3.cond, label %exit, label %loop.4, !prof !0
>>>> +
>>>> +; CHECK-NEXT: loop.4: float = 0.0,
>>>> +loop.4:
>>>> +  %exit.4.cond = call i1 @foo()
>>>> +  br i1 %exit.4.cond, label %exit, label %loop, !prof !0
>>>> +
>>>> +; CHECK-NEXT: exit: float = 1.0, int = [[ENTRY]]
>>>> +exit:
>>>> +  ret void
>>>> +}
>>>> +
>>>> +declare i1 @foo()
>>>> +
>>>> +!0 = metadata !{metadata !"branch_weights", i32 4294967295, i32 1}
>>>> 
>>>> 
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>> 
>