[PATCH] Infer known bits from dominating conditions

[Philip Reames]

On 02/19/2015 05:47 AM, hfinkel at anl.gov wrote:
> Thanks for working on this!
Thanks for reviewing!

I'm going to try to get an updated version posted today.  I want to 
respond to a few comments first.  Assume anything I don't comment on 
will be fixed in the updated diff.
>
> I'd really like to see a compile-time comparison with an implementation that just walks up the dominator tree. It seems like that technique would be more powerful (because it could catch conditions where the variable is not a direct argument to the icmp). It might be manageable (with a cutoff for really deep trees, and noting that we can stop searching after we reach the definition of the variable itself).
To be clear, is this a request for me to evaluate this before 
submission?  I really don't see the need given that a) the current 
implementation is fast, and b) this is an experimental flag.
>
> Also, full-context patches please: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
>
>
> ================
> Comment at: lib/Analysis/ValueTracking.cpp:42
> @@ -41,1 +41,3 @@
>   
> +static cl::opt<bool> EnableDomConditions("value-tracking-dom-conditions", cl::init(false));
> +
> ----------------
> Line too long?
>
> ================
> Comment at: lib/Analysis/ValueTracking.cpp:498
> @@ +497,3 @@
> +  // Avoid massive lists
> +  if (V->hasNUsesOrMore(20))
> +    return;
> ----------------
> Please make this cutoff a command-line parameter.
>
> ================
> Comment at: lib/Analysis/ValueTracking.cpp:510
> @@ +509,3 @@
> +    if (!Cmp ||
> +        !Cmp->hasOneUse())
> +      continue;
> ----------------
> Why do you care that the cmp has only one use? Why not look at all of the branches that use the cmp result?
We could, and may someday.  Doing so involves a potential increase in 
compile time.  I'm deliberately going for minimal compile time impact, 
even at the loss of some effectiveness.  I want to be able to turn this 
on by default.  Most of my actual motivating cases are really simple.

I would strongly prefer going with the fastest implementation now and 
evaluating extensions at a later time.  If you think it matters and is 
cheap enough, feel free to contribute a patch.  :)
>
> ================
> Comment at: lib/Analysis/ValueTracking.cpp:541
> @@ +540,3 @@
> +      // TODO: implement unsigned bound from below (known one bits)
> +      // TODO: common condition check implementations with assumes
> +      // TODO: implement other patterns from assume (e.g. V & B == A)
> ----------------
> Indeed, can't you share all of that logic that is used by the assume implementation?
We can share this code, but it'll be a non-trivial refactoring.  In 
particular, the assume code is more general than I want in this code 
path due to the compile time issues.  I would need to find a way to 
abstract over that generality.

I will probably do this after submission, but I really don't want to do 
it now.
>
> Although to really do that you'd have to start with the dominating branches, walking up?
Nope.  It can work with the current approach too.
>
> http://reviews.llvm.org/D7708
>
> EMAIL PREFERENCES
>    http://reviews.llvm.org/settings/panel/emailpreferences/
>
>