[PATCH] Infer known bits from dominating conditions

[Philip Reames]

Responding to Sanjoy's review comments.


================
Comment at: lib/Analysis/ValueTracking.cpp:498
@@ +497,3 @@
+  // Avoid massive lists
+  if (V->hasNUsesOrMore(20))
+    return;
----------------
sanjoy wrote:
> hfinkel wrote:
> > Please make this cutoff a command-line parameter.
> I remember hearing somewhere that the walking the use-list in LLVM is cache-miss'y.  If (a big if) that is true, then for variables with a large number of uses you're going to spend a lot of time just skimming through the use list here.  To that end, does it make sense to have this cutoff in the loop where you're looping through the use list?
This hasn't been a measurable impact.  I'd be a bit leery of effecting the code generation of the loop as well.  Besides, once somethings in cache (from the first check), accessing it again (in the loop) is cheap.  

================
Comment at: lib/Analysis/ValueTracking.cpp:510
@@ +509,3 @@
+    if (!Cmp ||
+        !Cmp->hasOneUse()) 
+      continue;
----------------
hfinkel wrote:
> Why do you care that the cmp has only one use? Why not look at all of the branches that use the cmp result?
Compile time.  I want to avoid walking potentially long use lists.  I'm matching specifically the common pattern of a branch controlled by a comparison right before it.  See my comment to Hal about possible future extensions.  

================
Comment at: lib/Analysis/ValueTracking.cpp:529
@@ +528,3 @@
+    BasicBlock *BB0 = BI->getSuccessor(0);
+    if (nullptr == BB0->getUniquePredecessor())
+      continue;
----------------
sanjoy wrote:
> I don't think this will work if `BI->getSuccessor(0) == BI->getSuccessor(1)`.  You need to check if the `false` branch of `BI` actually branches to something other than `BB0`.
You're right.  I need to account for that case.  Thanks for finding this!

================
Comment at: lib/Analysis/ValueTracking.cpp:531
@@ +530,3 @@
+      continue;
+    if (!Q.DT->dominates(BB0, Cxt->getParent()))
+      continue;
----------------
sanjoy wrote:
> The only other place that uses `Q.DT` checks to see if it isn't `nullptr`.  Is there reason to believe that we actually have the dominator tree?
It's checked at the top of the function.  We do nothing if we don't have a dom tree.  

================
Comment at: lib/Analysis/ValueTracking.cpp:533
@@ +532,3 @@
+      continue;
+    
+    Value *LHS = Cmp->getOperand(0);
----------------
sanjoy wrote:
> Actually, it might be cleaner and more powerful to just use the `BasicBlockEdge` version of `dominates` here.  `dominates` will also handle the case where `BB0` has multiple preds but `BB0` dominates all of them.
This is a good idea.  Hadn't thought of that interface.  Will definitely have to look into that.

================
Comment at: lib/Analysis/ValueTracking.cpp:574
@@ +573,3 @@
+        computeKnownBits(RHS, KnownZeroTemp, KnownOneTemp, TD, Depth+1, Q);
+        // Whatever high bits in c are zero are known to be zero (if c is a
+        // power of 2, then one more).
----------------
sanjoy wrote:
> Nit: should this be 'high bits in RHS'?
Will fix.

http://reviews.llvm.org/D7708

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