[llvm-commits] [llvm] r80715 - in /llvm/trunk: include/llvm/Analysis/ProfileInfoLoader.h lib/Analysis/ProfileInfoLoader.cpp lib/Analysis/ProfileInfoLoaderPass.cpp

[Andreas Neustifter]

Hi,

Daniel Dunbar wrote:
> Hi Andreas,
> 
> One minor comment,
> 
> On Tue, Sep 1, 2009 at 12:08 PM, Andreas Neustifter<astifter at gmx.at> wrote:
>> -  // Make sure we have enough space...
>> +  // Make sure we have enough space... The space is initialised to -1 to
>> +  // facitiltate the loading of missing values for OptimalEdgeProfiling.
>>   if (Data.size() < NumEntries)
>> -    Data.resize(NumEntries);
>> +    Data.resize(NumEntries, -1);
> 
> FYI I changed this to ~0U to fix a build warning.

Thanks!

>>   // Accumulate the data we just read into the data.
>>   if (!ShouldByteSwap) {
>> -    for (unsigned i = 0; i != NumEntries; ++i)
>> -      Data[i] += TempSpace[i];
>> +    for (unsigned i = 0; i != NumEntries; ++i) {
>> +      unsigned data = TempSpace[i];
>> +      if (data != (unsigned)-1) {       // only load data if its not MissingVal
>> +        if (Data[i] == (unsigned)-1) {
>> +          Data[i] = data;               // if data is still initialised
>> +        } else {
>> +          Data[i] += data;
>> +        }
>> +      }
>> +    }
>>   } else {
>> -    for (unsigned i = 0; i != NumEntries; ++i)
>> -      Data[i] += ByteSwap(TempSpace[i], true);
>> +    for (unsigned i = 0; i != NumEntries; ++i) {
>> +      unsigned data = ByteSwap(TempSpace[i], true);
>> +      if (data != (unsigned)-1) {       // only load data if its not MissingVal
>> +        if (Data[i] == (unsigned)-1) {
>> +          Data[i] = data;
>> +        } else {
>> +          Data[i] += data;
>> +        }
>> +      }
>> +    }
> 
> This would be simpler if there was a:
> --
> static const unsigned AddCounts(unsigned A, unsigned B) {
>   // If either value is undefined, use the other.
>   if (A == ~0U) return B;
>   if (B == ~0U) return A;
>   return A + B;
> }
> --
> helper earlier, then it becomes obvious what the loops are doing:
> --
>     for (unsigned i = 0; i != NumEntries; ++i)
>       Data[i] = AddCounts(Data[i], TempSpace[i]);
> --

I found this code _very_ ugly myself, is was planing on cleaning it up today, thanks for the hints!

> Do these values need to become uint64_t at some point?

Was thinking about that because there is no gurantee that the counters do not overflow. But this would be a major change, in the complete tool chain, but I will look into this.

Andi