[llvm-commits] [llvm] r142751 - in /llvm/trunk: include/llvm/Support/BranchProbability.h lib/Analysis/BranchProbabilityInfo.cpp lib/Support/BranchProbability.cpp unittests/Support/BlockFrequencyTest.cpp

[Benjamin Kramer]

On 23.10.2011, at 21:35, Chandler Carruth wrote:

> After some discussion on IRC...
> 
> On Sun, Oct 23, 2011 at 4:19 AM, Benjamin Kramer <benny.kra at googlemail.com> wrote:
> +  int64_t compare(BranchProbability RHS) const {
> +    return (uint64_t)N * RHS.D - (uint64_t)D * RHS.N;
> +  }
> 
> It would be good to comment here that the result of the LHS of the subtract can't be larger than INT64_MAX without the RHS being large enough to reduce it because N <= D. It might also be nice to just assert that fact.
>  
> --- llvm/trunk/unittests/Support/BlockFrequencyTest.cpp (original)
> +++ llvm/trunk/unittests/Support/BlockFrequencyTest.cpp Sun Oct 23 06:19:14 2011
> @@ -53,4 +53,24 @@
>   EXPECT_EQ(Freq.getFrequency(), UINT64_MAX);
>  }
> 
> +TEST(BlockFrequencyTest, ProbabilityCompare) {
> +  BranchProbability A(4, 5);
> +  BranchProbability B(4U << 29, 5U << 29);
> 
> Maybe add a test case that compares B against a RHS with a similarly large denominator value to ensure that overflow of INT64_MAX is indeed brought back down in the computation? In particular the interesting one to my mind is (UINT32_MAX, UINT32_MAX) vs. (1, UINT32_MAX).

I could come up with a case where the subtraction gives wrong results. UINT32_MAX/UINT32_MAX is bigger than 0/UINT32_MAX but UINT32_MAX*UINT32_MAX - UINT32_MAX*0 is still negative. Fixed in r142794.

Thanks for reviewing this!
- Ben