[llvm-commits] [llvm] r132613 - in /llvm/trunk: include/llvm/Analysis/BranchProbabilityInfo.h include/llvm/InitializePasses.h lib/Analysis/Analysis.cpp lib/Analysis/BranchProbabilityInfo.cpp lib/Analysis/CMakeLists.txt

[Andrew Trick]

On Jun 3, 2011, at 7:29 PM, Alistair Lynn <arplynn at gmail.com> wrote:

> Hi-
> 
> Given that
> 
>> +  // Returned value is between 1 and UINT_MAX. Look at BranchProbabilityInfo.cpp
>> +  // for details.
> 
> Is this function not prone to integer overflow issues?
> 
>> +// TODO: This currently hardcodes 80% as a fraction 4/5. We will soon add a
>> +// BranchProbability class to encapsulate the fractional probability and
>> +// define a few static instances of the class for use as predefined thresholds.
>> +bool BranchProbabilityInfo::isEdgeHot(BasicBlock *Src, BasicBlock *Dst) const {
>> +  unsigned Sum = 0;
>> +  for (succ_iterator I = succ_begin(Src), E = succ_end(Src); I != E; ++I) {
>> +    BasicBlock *Succ = *I;
>> +    unsigned Weight = getEdgeWeight(Src, Succ);
>> +    unsigned PrevSum = Sum;
>> +
>> +    Sum += Weight;
>> +    assert(Sum > PrevSum); (void) PrevSum;
>> +  }
>> +
>> +  return getEdgeWeight(Src, Dst) * 5 > Sum * 4;
>> +}
> 
> The sum itself is checked for overflow in the assert, but surely there's nothing preventing overflow on the multiplies on the condition at the end?
> 
> Alistair

Thanks for the review. Jakub is planning to implement multiplication by branch probability as 64 bit multiply with overflow check. This case should be fixed soon. 

FYI. The design doc expresses our intent to avoid numerical issues but we probably won't have it perfect in the first patch set. Still it's good to keep pointing out discrepancies.

Jakub, feel free to use FIXME comments in places that need more work. 

Andy