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

[Michael Gottesman]

(when I said this is incorrect, I mean that letting it through and setting it to any value is incorrect, so I believe we are both incorrect).

Michael

On Dec 15, 2013, at 8:41 PM, Michael Gottesman <mgottesman at apple.com> wrote:

> 
> On Dec 15, 2013, at 2:44 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
>> On 2013 Dec 15, at 13:58, Michael Gottesman <mgottesman at apple.com> wrote:
>> 
>>> The comment could be made clearer clearing up those issues. I am going to fix up those comments in a bit.
>> 
>> Thanks!  It might be enough to list the input and expected output of the
>> degenerate cases and let the reader confirm the logic.
>> 
>>> I was avoiding the unnecessary branch. I think with a fixed up comment this will be clear.
>> 
>> Ah -- I’d missed that the branch in std::min() gets optimized away.
>> 
>>> I have no problem adding these test cases.
>> 
>> I thought of another degenerate case, and unfortunately I think the
>> current code gets the wrong answer:
>> 
>> - APInt(1, 1).nearestLogBase2() == 0 // lg(1) should give 0, not 1.
> 
> This is incorrect since log2 is not a well defined operation in Z/Z_1. If one defines log2(x) as the largest N such that 2^N < x then since in Z/Z_1, 2^N = 0^N, N could be any number. In reality we should probably add in an assertion to make sure that the routines are only called if the BitWidth is > 1.
> 
> Michael
> 
>> 
>> Duncan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131215/94a33830/attachment.html>