[PATCH] D15232: [InstCombine] Aggressively fold compares that can be discovered to be constant

[Philip Reames via llvm-commits]

On 12/07/2015 02:16 AM, James Molloy wrote:
> Hi Philip,
>
> I've taken a closer look at this now. There are several problems with LVI:
>
>   1. getValueAt() is not as powerful as getValueAtEnd(). I think this 
> is an oversight - it looks like the intention is that getValueAt() 
> should be more powerful than getValueAtEnd() because it has the 
> ability to use assume intrinsics and range metadata. However, if the 
> range metadata and assume intrinsics give no answer (undefined), we 
> don't fall back on our other analyses (getBlockValue()).
This is a known (to me at least) problem.  I've got a series of patches 
stuck in review right now which will eventually fix this.
>   2. LVI does not look through select instructions.
Again, I have a patch ready once review gets unblocked.  (To be clear, I 
think the next unblocking step is mine; the cycle time is just long 
enough to get it out of my active list.)
>   3. LVI does not handle loop PHIs at all.
I'm not sure what you're describing here.  LVI can prove limited 
recursive conditions.  What are you trying to describe here?  Do you 
have a specific example case?
>
> I have patches for (1) and (2) and will send them for review 
> irrespective of what happens with this review, as I think they're 
> obviously good things to do. I think (3) is the real deal breaker - my 
> motivating example has this chain of selects threaded over a loop, and 
> LVI just cannot deal with loops at all.
>
> I think I recall someone (Jiangning Liu?) attempting to add support 
> for loops to LVI, but having difficulty retaining the lattice 
> structure and making sure it always converged.
Yep, that would probably be challenging.  It does become easier once my 
"intersect" patch lands - since it can be a separable analysis which is 
then intersected - but it's not a small project.
>
> So at the moment I'm not sure how else to implement this other than 
> what I've already uploaded - even pushing the icmp queries up the 
> use-def graph would eventually end up at the head of a loop - 
> somewhere we need to do some analysis that can pass through loop PHIs.
Ok, thank you for looking at the options.  I'm still a bit unhappy about 
the chosen approach, but with your explanation I see why you settled on 
the given approach.  I withdraw my objections.
>
> Cheers,
>
> James
>
> On Sat, 5 Dec 2015 at 19:34 James Molloy <james at jamesmolloy.co.uk 
> <mailto:james at jamesmolloy.co.uk>> wrote:
>
>     Hi Philip,
>
>     Thanks for taking a look!
>
>     I agree that searching an arbitrary size value graph would be
>     horrendously expensive. What I have implemented is to search
>     through unlimited selects and phis (should be cheap, these aren't
>     commonly chained deeply, see GetUnderlyingObjects), and will look
>     through only a certain number of binary operators. The default
>     depth is 1, which will go through one add but no more
>     (configurable by the user). I think overall this should restrict
>     the potential for bloat.
>
>     I have a motivating example which is a chain of selects and phis
>     in a loop. The loop part is important - it's what seems to confuse
>     instcombine currently. What I can do is take a look to see what's
>     causing CVP / LVI to not support this case and report back. Thanks
>     for the hint!
>
>     I'm not sure the last approach you suggested would work for the
>     loop use case, although I haven't thought majorly hard about it
>     just yet.
>
>     Thanks again,
>
>     James
>     On Fri, 4 Dec 2015 at 20:36, Philip Reames via llvm-commits
>     <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>>
>     wrote:
>
>         reames added a subscriber: reames.
>         reames added a comment.
>
>         At a high level, I'm a bit unsure about the formulation chosen
>         here.  I'm worried that seeking to find a set of constant
>         values through a potentially large value graph might be expensive.
>
>         A couple of specific questions:
>
>         - The examples you've listed should be entirely handled by
>         LazyValueInfo and either JumpThreading or CVP.  Do you have a
>         strong reason InstCombine needs to perform the same optimization?
>         - Have you considered phrasing this as pushing the query (i.e.
>         icmp) back along the inputs?  Doing this might a) let you
>         terminate earlier or b) unswitch the comparison if the select
>         is only used by the compare.
>
>
>         http://reviews.llvm.org/D15232
>
>
>
>         _______________________________________________
>         llvm-commits mailing list
>         llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>         http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>

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