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

Mon Jun 24 10:38:58 PDT 2013

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

That makes sense, thanks!

 -Hal

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

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory