[llvm] r289755 - Make processing @llvm.assume more efficient by using operand bundles

[Hal Finkel via llvm-commits]

On 01/03/2017 10:01 AM, Daniel Berlin wrote:
>
>
> On Tue, Jan 3, 2017 at 5:43 AM, Hal Finkel via llvm-commits 
> <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>
>     Hi Philip,
>
>     First, I'm planning on rewriting this change to be more targeted,
>     specifically to take the alternative approach we discussed briefly
>     on the review thread and keep the list of affected variables in
>     the assumption cache.
>
>
>     Second, regarding motivation, the problem is not the basic-block
>     scan (with the assumption cache, we actually do very little of
>     that anyway). The core problem is that, in ValueTracking, which is
>     used heavily by InstCombine and other passes, checks all
>     assumptions in each function for every variable being processed to
>     determine known bits, etc. These checks are pattern matches, and
>     while they normally fail quickly, as the number of assumptions
>     increases, this is a large problem: it makes known-bits
>     determination O(#assumptions in the function), for every variable,
>     in the common case, and, because the assumption processing is part
>     of the recursive known-bits procedure, the constant factor can be
>     large too given certain kinds of inputs. What we need for
>     ValueTracking (and also for LVI and maybe others) is a way to
>     cache which variables are (usefully) affected by which assumptions.
>
>
> When you say affected, do you mean "what uses"?
> Because your operand bundles change, does not really do that.
> For a given use I still have to find the assume (which is in the 
> middle of a block), then go to the bundle, then look at the uses of 
> the things in that bundle
> Or am i missing something?

You're missing something. The "affected uses" are added by InstCombine 
by looking for patterns useful to ValueTracking, etc. So, for example, 
if we have:

    %cmp1 = icmp eq i8 %a, 5
    call void @llvm.assume(i1 %cmp1)

Then InstCombine would come along and add the operand bundle like this:

    call void @llvm.assume(i1 %cmp1) [ "affected"(i8 %a) ]

So, if we're looking at %a, and want to know if there's some assumption 
that might be relevant to it, we just need to look at %a's users 
(instead of looking through all assumptions).

It's certainly true that, as discussed below, once we find an assume in 
the users of %a, we need to verify the dominance relationship between 
the assume and %a, but this is still rare and has not yet presented 
itself as a problem as far as I can tell. The first problem is that 
checking whether some variable, %x, is potentially affected by an assume 
cannot be O(#assumes in the function). That's what ValueTracking, LVI, 
etc. do now and, for obvious reasons, causes problems as we make 
more-extensive use of assumes (or don't because of this performance 
problem).

>
>     Regarding your suggestions below, I can definitely see how we
>     might run into a problem with the dominance checking if we had a
>     large number of assumptions which affected a variable -- it is
>     only when we have an assumption that potentially affects a
>     variable that we do any of the dominance checking -- but I've not
>     run into any such cases yet.
>
>
> Note: I have precisely the same problem with newgvn's predicate handling:
>
> I need to be able to tell, for a given instruction, what predicates 
> dominate it (including assumes)
>
> It's pretty fast for most predicates, because they occur at the end of 
> blocks, but could still be sped up.
> For assumes, not so much, because they can occur wherever.
>
> I have a prototype where i've implemented e-ssa to solve both 
> problems. It requires the introduction of one intrinsic, and some use 
> renaming.

Where do we introduce the intrinsic? Do we maintain it throughout the 
pipeline?

>
> IE
> if (foo == 50) {
> } else { }
>
> becomes
>
> if (foo == 50) {
> foo1 = predicateinfo(foo, the icmp of foo )
> (use foo1)
> } else {
> foo2 = predicateinfo(foo, the icmp of foo)
> (use foo2)
> }
>
> Once that's done, finding the set of predicates that usefully affect a 
> given use is trivial, as you can just walk the def-use chains.
>
> I haven't implemented it for assume, but it should work just as well 
> for that.
>
> I'm somewhat against caching solutions and hacky here,I generally 
> prefer to solve infrastructury problems with better infrastructure.  
> Caching tends to just kick the can down the road, and often makes it 
> harder to build better infrastructure.  It makes sense when the change 
> is significantly harder to get right, and not worth the cost. Here, 
> honestly, we can do better.
> We have plenty of cases where CVP, etc, will be significantly sped up 
> by the above (i can hand them to you) but not by your change.
>
> Note also that it improves a large set of passes.
> For example, the variable split enables SCCP to do correlated value 
> propagation (trivially for equality comparison, other things with a 
> bit of work)
>
> Before, it could only do something if "foo" turned out the same 
> everywhere.
> After, you can propagate  "foo == 50" to the foo1 branch.
>
> Early measurements come out to a few percent more constants found total.

How will we know when to add the predicateinfo intrinsic, or will we do 
it for all values used in the conditionally-executed region? This patch 
did the equivalent thing by pattern-matching the condition.

Generally, I'm in favor of going in this direction.

For assumes, we'd still need to pattern match, right? If so, this is 
still a cache in a sense ;) -- I suspect we'd still need the assumption 
cache too in order to efficiently compute ephemeral values (so that the 
inline cost analysis, etc. still have the right complexity bounds).

Thanks again,
Hal

>
>

-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170104/f279b2c5/attachment.html>