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

[Duncan P. N. Exon Smith]

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.

> +    // 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.

> +    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];

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