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

Tue Dec 8 08:52:16 PST 2015

Thanks Philip,

David, do you have any more comments on this?

Cheers,

James

On Mon, 7 Dec 2015 at 18:41 Philip Reames <listmail at philipreames.com> wrote:

> 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> 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> 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
>>> 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/20151208/86bf2f5e/attachment.html>