[llvm] r197272 - [block-freq] Add the method APInt::nearestLogBase2().

[Michael Gottesman]

On Dec 15, 2013, at 10:06 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:

> Hi Michael,
> 
> On 2013 Dec 13, at 12:47, Michael Gottesman <mgottesman at apple.com> wrote:
>> +  /// \returns the nearest log base 2 of this APInt. Ties round up.
>> +  unsigned nearestLogBase2() const {
>> +    // This is implemented by taking the normal log 2 of a number and adding 1
>> +    // to it if MSB - 1 is set.
> 
> Your use of MSB here is confusing, since MSB == BitWidth - 1 and
> MSB - 1 == BitWidth - 2.  I think the following is more accurate:
> 
> // This is implemented by taking logBase2() and adding 1 if the
> // logBase2()-1 bit is set.

When I said MSB I meant most significant set bit (it is what I was thinking of). I am going to revamp the comments there in to fix that up.

> 
>> +    // We follow the model from logBase2 that logBase2(0) == UINT32_MAX. This
>> +    // works since if we have 0, MSB will be 0. Then we subtract one yielding
>> +    // UINT32_MAX. Finally extractBit of MSB - 1 will be UINT32_MAX implying
>> +    // that we get BitWidth - 1.
> 
> This comment is confusing.  The prime culprits are the references
> to MSB and (the now deleted) extractBit function, but the logic is
> tough to follow regardless.

The comment could be made clearer clearing up those issues. I am going to fix up those comments in a bit.

> 
>> +    unsigned lg = logBase2();
>> +    return lg + unsigned(extractBit(std::min(lg - 1, BitWidth - 1)));
> 
> Is the following naive code somehow slower?  I think it’s easier
> to understand (and document).
> 
> unsigned lg = logBase2();
> // Don’t check the lg - 1 bit in degenerate cases.
> if (lg - 1 >= BitWidth)
>  return lg;
> return lg + (*this)[lg - 1];

I was avoiding the unnecessary branch. I think with a fixed up comment this will be clear.

> 
>> +TEST(APIntTest, nearestLogBase2) {
> 
> I think your tests miss the most interesting corners, i.e.:
> 
> - APInt(BitWidth, 0).nearestLogBase2() == UINT32_MAX
> - APInt(BitWidth, 1).nearestLogBase2() == 0
> - APInt(BitWidth, -1).nearestLogBase2() == BitWidth
> 
> The first two, in particular, require special handling to prevent
> invalid accesses into APInt.

I have no problem adding these test cases.

Michael