[PATCH] Infer known bits from dominating conditions

Sanjoy Das sanjoy at playingwithpointers.com
Wed Feb 25 18:37:43 PST 2015


Comments inline.


================
Comment at: lib/Analysis/ValueTracking.cpp:498
@@ +497,3 @@
+  // Avoid massive lists
+  if (V->hasNUsesOrMore(20))
+    return;
----------------
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?

================
Comment at: lib/Analysis/ValueTracking.cpp:508
@@ +507,3 @@
+  for (auto U : V->users()) {
+    ICmpInst* Cmp = dyn_cast<ICmpInst>(U);
+    if (!Cmp ||
----------------
Nit: should be `ICmpInst *Cmp`.  In general, I'd suggest just running this patch through `clang-format` before checking in.

================
Comment at: lib/Analysis/ValueTracking.cpp:529
@@ +528,3 @@
+    BasicBlock *BB0 = BI->getSuccessor(0);
+    if (nullptr == BB0->getUniquePredecessor())
+      continue;
----------------
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`.

================
Comment at: lib/Analysis/ValueTracking.cpp:531
@@ +530,3 @@
+      continue;
+    if (!Q.DT->dominates(BB0, Cxt->getParent()))
+      continue;
----------------
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?

================
Comment at: lib/Analysis/ValueTracking.cpp:533
@@ +532,3 @@
+      continue;
+    
+    Value *LHS = Cmp->getOperand(0);
----------------
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.

================
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).
----------------
Nit: should this be 'high bits in RHS'?

http://reviews.llvm.org/D7708

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






More information about the llvm-commits mailing list