[PATCH] [ValueTracking] Fix PR23011.

[Sanjoy Das]

================
Comment at: lib/Analysis/ValueTracking.cpp:1937
@@ +1936,3 @@
+      // the number of sign bits is MIN(CLZ(C-1), CLO(-C+1)).
+      //
+
----------------
nicholas wrote:
> Blank comment line.
Will fix.

================
Comment at: lib/Analysis/ValueTracking.cpp:1941
@@ -1935,1 +1940,3 @@
+      unsigned WhenNegative = (-(*Denominator) + 1).countLeadingOnes();
+      unsigned ResBits = std::min(WhenPositive, WhenNegative);
 
----------------
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?

================
Comment at: test/Analysis/ValueTracking/pr230011.ll:1
@@ +1,2 @@
+; RUN: opt -indvars -S < %s | FileCheck %s
+
----------------
nicholas wrote:
> File name is off "pr230011.ll" should be "pr23011.ll".
> 
> Also, if this is a bug in value tracking, isn't there a more direct way to observe it than this? Maybe just srem + and and then opt -instsimplify?
Yes.  I think I'll also switch out the constant-foldable instruction with something like `srem %foo, 3`.

http://reviews.llvm.org/D8600

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