[llvm] r184590 - Revert "BlockFrequency: Saturate at 1 instead of 0 when multiplying a frequency with a branch probability."

[Benjamin Kramer]

On 23.06.2013, at 15:07, Hal Finkel <hfinkel at anl.gov> wrote:

> ----- Original Message -----
>> 
>> On 23.06.2013, at 07:50, Chandler Carruth < chandlerc at google.com >
>> wrote:
>> 
>> 
>> 
>> 
>> 
>> I don't think this had anything specifically to do with PPC.
>> Saturating at 1 causes the fundamental mathematical model of block
>> frequencies to break down and do all kinds of strange things
>> (including triggering assertions).
>> 
>> 
>> Right. The PPC self host bot just happened to be the first one that
>> failed.
> 
> Thanks! So 1 is problematic and 0 is not?

It asserts that frequencies going out of a loop are less or equal to the frequency that went into the loop. This can be easily broken when saturating to 1. You can have many blocks that have a zero frequency due to underflows; If you saturate at 1 and take the sum the result becomes larger. Over the whole function this can yield values that are way off from what we really wanted.

This seems to be a fundamental problem of representing block frequencies as a fixed point number. We'll probably need a more flexible way. Chandler suggested to store it as a ratio and rescale when necessary to minimize precision loss.

- Ben

> 
> -Hal
> 
>> 
>> 
>> - Ben
>> 
>> 
>> 
>> 
>> 
>> On Sat, Jun 22, 2013 at 9:51 PM, Hal Finkel < hfinkel at anl.gov >
>> wrote:
>> 
>> 
>> 
>> ----- Original Message -----
>>> Author: d0k
>>> Date: Fri Jun 21 15:20:27 2013
>>> New Revision: 184590
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=184590&view=rev
>>> Log:
>>> Revert "BlockFrequency: Saturate at 1 instead of 0 when multiplying
>>> a
>>> frequency with a branch probability."
>>> 
>>> This reverts commit r184584. Breaks PPC selfhost.
>> 
>> That seems odd. Which build was this? Do you have any additional
>> details?
>> 
>> Thanks again,
>> Hal
>> 
>> 
>> 
>>> 
>>> Removed:
>>> llvm/trunk/test/Analysis/BlockFrequencyInfo/singularity.ll
>>> Modified:
>>> llvm/trunk/include/llvm/Support/BlockFrequency.h
>>> llvm/trunk/lib/Support/BlockFrequency.cpp
>>> llvm/trunk/unittests/Support/BlockFrequencyTest.cpp
>>> 
>>> Modified: llvm/trunk/include/llvm/Support/BlockFrequency.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/BlockFrequency.h?rev=184590&r1=184589&r2=184590&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/Support/BlockFrequency.h (original)
>>> +++ llvm/trunk/include/llvm/Support/BlockFrequency.h Fri Jun 21
>>> 15:20:27 2013
>>> @@ -30,20 +30,12 @@ class BlockFrequency {
>>> public:
>>> BlockFrequency(uint64_t Freq = 0) : Frequency(Freq) { }
>>> 
>>> - /// \brief Returns the frequency of the entry block of the
>>> function.
>>> static uint64_t getEntryFrequency() { return ENTRY_FREQ; }
>>> -
>>> - /// \brief Returns the frequency as a fixpoint number scaled by
>>> the entry
>>> - /// frequency.
>>> uint64_t getFrequency() const { return Frequency; }
>>> 
>>> - /// \brief Multiplies with a branch probability. The computation
>>> will never
>>> - /// overflow. If the result is equal to zero but the input wasn't
>>> this method
>>> - /// will return a frequency of one.
>>> BlockFrequency &operator*=(const BranchProbability &Prob);
>>> const 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;
>>> 
>>> 
>>> Modified: llvm/trunk/lib/Support/BlockFrequency.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/BlockFrequency.cpp?rev=184590&r1=184589&r2=184590&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/Support/BlockFrequency.cpp (original)
>>> +++ llvm/trunk/lib/Support/BlockFrequency.cpp Fri Jun 21 15:20:27
>>> 2013
>>> @@ -65,9 +65,6 @@ uint64_t div96bit(uint64_t W[2], uint32_
>>> 
>>> 
>>> BlockFrequency &BlockFrequency::operator*=(const BranchProbability
>>> &Prob) {
>>> - if (Frequency == 0)
>>> - return *this;
>>> -
>>> uint32_t n = Prob.getNumerator();
>>> uint32_t d = Prob.getDenominator();
>>> 
>>> @@ -87,15 +84,10 @@ BlockFrequency &BlockFrequency::operator
>>> // 64-bit.
>>> mult96bit(Frequency, n, W);
>>> Frequency = div96bit(W, d);
>>> - } else {
>>> - // Fast case.
>>> - Frequency = mulRes / d;
>>> + return *this;
>>> }
>>> 
>>> - // Limit the result to 1; 0 is a sentinel value. This keeps
>>> BlockFrequencyInfo
>>> - // from getting stuck at zero frequencies just because a value
>>> became too
>>> - // small to be represented as a BlockFrequency.
>>> - Frequency = (n == 0 || Frequency != 0) ? Frequency : 1;
>>> + Frequency = mulRes / d;
>>> return *this;
>>> }
>>> 
>>> 
>>> Removed: llvm/trunk/test/Analysis/BlockFrequencyInfo/singularity.ll
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/BlockFrequencyInfo/singularity.ll?rev=184589&view=auto
>>> ==============================================================================
>>> --- llvm/trunk/test/Analysis/BlockFrequencyInfo/singularity.ll
>>> (original)
>>> +++ llvm/trunk/test/Analysis/BlockFrequencyInfo/singularity.ll
>>> (removed)
>>> @@ -1,65 +0,0 @@
>>> -; RUN: opt < %s -analyze -block-freq | FileCheck %s
>>> -; PR16402
>>> -
>>> -define void @test1(i32 %n) nounwind {
>>> -entry:
>>> - %call = tail call i32* @cond() nounwind
>>> - %tobool = icmp eq i32* %call, null
>>> - br i1 %tobool, label %land.lhs.true, label %if.end
>>> -
>>> -land.lhs.true: ; preds = %entry
>>> - %call1 = tail call i32* @cond() nounwind
>>> - %tobool2 = icmp eq i32* %call1, null
>>> - br i1 %tobool2, label %land.lhs.true3, label %if.end
>>> -
>>> -land.lhs.true3: ; preds =
>>> %land.lhs.true
>>> - %call4 = tail call i32* @cond() nounwind
>>> - %tobool5 = icmp eq i32* %call4, null
>>> - br i1 %tobool5, label %land.lhs.true6, label %if.end
>>> -
>>> -land.lhs.true6: ; preds =
>>> %land.lhs.true3
>>> - %call7 = tail call i32* @cond() nounwind
>>> - %tobool8 = icmp eq i32* %call7, null
>>> - br i1 %tobool8, label %land.lhs.true9, label %if.end
>>> -
>>> -land.lhs.true9: ; preds =
>>> %land.lhs.true6
>>> - %call10 = tail call i32* @cond() nounwind
>>> - %tobool11 = icmp eq i32* %call10, null
>>> - br i1 %tobool11, label %land.lhs.true12, label %if.end
>>> -
>>> -land.lhs.true12: ; preds =
>>> %land.lhs.true9
>>> - %call13 = tail call i32* @cond() nounwind
>>> - %tobool14 = icmp eq i32* %call13, null
>>> - br i1 %tobool14, label %land.lhs.true15, label %if.end
>>> -
>>> -land.lhs.true15: ; preds =
>>> %land.lhs.true12
>>> - %call16 = tail call i32* @cond() nounwind
>>> - %tobool17 = icmp eq i32* %call16, null
>>> - br i1 %tobool17, label %for.cond.preheader, label %if.end
>>> -
>>> -for.cond.preheader: ; preds =
>>> %land.lhs.true15
>>> - %cmp21 = icmp eq i32 %n, 0
>>> - br i1 %cmp21, label %for.end, label %for.body
>>> -
>>> -for.body: ; preds =
>>> %for.cond.preheader, %for.body
>>> - %i.022 = phi i32 [ %inc, %for.body ], [ 0, %for.cond.preheader ]
>>> - %call18 = tail call i32 @call() nounwind
>>> - %inc = add nsw i32 %i.022, 1
>>> - %cmp = icmp eq i32 %inc, %n
>>> - br i1 %cmp, label %for.end, label %for.body
>>> -
>>> -for.end: ; preds =
>>> %for.body, %for.cond.preheader
>>> - %call19 = tail call i32* @cond() nounwind
>>> - br label %if.end
>>> -
>>> -if.end: ; preds =
>>> %land.lhs.true15, %land.lhs.true12, %land.lhs.true9,
>>> %land.lhs.true6, %land.lhs.true3, %land.lhs.true, %entry, %for.end
>>> - ret void
>>> -
>>> -; CHECK: entry = 1024
>>> -; CHECK-NOT: for.body = 0
>>> -; CHECK-NOT: for.end = 0
>>> -}
>>> -
>>> -declare i32* @cond() nounwind
>>> -
>>> -declare i32 @call() nounwind
>>> 
>>> Modified: llvm/trunk/unittests/Support/BlockFrequencyTest.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/BlockFrequencyTest.cpp?rev=184590&r1=184589&r2=184590&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/unittests/Support/BlockFrequencyTest.cpp (original)
>>> +++ llvm/trunk/unittests/Support/BlockFrequencyTest.cpp Fri Jun 21
>>> 15:20:27 2013
>>> @@ -8,22 +8,11 @@ using namespace llvm;
>>> 
>>> namespace {
>>> 
>>> -TEST(BlockFrequencyTest, ZeroToZero) {
>>> - BlockFrequency Freq(0);
>>> - BranchProbability Prob(UINT32_MAX - 1, UINT32_MAX);
>>> - Freq *= Prob;
>>> - EXPECT_EQ(Freq.getFrequency(), 0u);
>>> -
>>> - Freq = 1;
>>> - Freq *= BranchProbability::getZero();
>>> - EXPECT_EQ(Freq.getFrequency(), 0u);
>>> -}
>>> -
>>> TEST(BlockFrequencyTest, OneToZero) {
>>> BlockFrequency Freq(1);
>>> BranchProbability Prob(UINT32_MAX - 1, UINT32_MAX);
>>> Freq *= Prob;
>>> - EXPECT_EQ(Freq.getFrequency(), 1u);
>>> + EXPECT_EQ(Freq.getFrequency(), 0u);
>>> }
>>> 
>>> TEST(BlockFrequencyTest, OneToOne) {
>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>> 
>> 
>> --
>> Hal Finkel
>> Assistant Computational Scientist
>> Leadership Computing Facility
>> Argonne National Laboratory
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>> 
> 
> -- 
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory