[PATCH] [ValueTracking] Fix PR23011.

[Nick Lewycky]

================
Comment at: lib/Analysis/ValueTracking.cpp:1941
@@ -1935,1 +1940,3 @@
+      unsigned WhenNegative = (-(*Denominator) + 1).countLeadingOnes();
+      unsigned ResBits = std::min(WhenPositive, WhenNegative);
 
----------------
sanjoy wrote:
> nicholas wrote:
> > Is this ever different from Denominator.ceilLogBase2()?
> The only case where this differs from `TyBits - ceilLogBase2` is when `Denominator` = `1` -- `TyBits - ceilLogBase2` will correctly be `TyBits` in that case (i.e. all bits are sign bits) but the current version of the code will return `0` (i.e. no bits are sign bits).  I noticed this just now, and it was not intentional.  I can think of two ways to fix this:
> 
>  * use `ceilLogBase2`.  I had decided to against this because I think it is easier to see why the analysis is correct in the current form (i.e. with CLZ/CLO).
> 
>  * special case WhenNegative (or the whole computation) when `Denominator` is `1`.
> 
> I'd prefer doing the latter.  WDYT?
I have a weak preference to keep the comment explaining it as it does, then to use ceilLogBase2 in the implementation. I like code reuse and don't think Denominator==1 should ever make it this far so I'd rather not special case it, but either way is just fine with me.

http://reviews.llvm.org/D8600

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/