[PATCH] Infer known bits from dominating conditions
listmail at philipreames.com
Tue Mar 3 10:10:48 PST 2015
Responding to Sanjoy's review comments.
Comment at: lib/Analysis/ValueTracking.cpp:498
@@ +497,3 @@
+ // Avoid massive lists
+ if (V->hasNUsesOrMore(20))
> 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 ||
> 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())
> 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 @@
+ if (!Q.DT->dominates(BB0, Cxt->getParent()))
> 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 @@
+ 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.
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).
> Nit: should this be 'high bits in RHS'?
More information about the llvm-commits