[llvm] r287585 - [InstCombine] canonicalize min/max constant to select's false value

[Sanjay Patel via llvm-commits]

A potential solution for this problem that would be more flexible and might
*improve* compile-time: extend simplifyICmpWithMinMax() with the
flexibility to create new instructions, and move it from InstSimplify to
InstCombine.

I had been operating under the assumption that folds that qualify as a
'simplify' always belong in InstSimplify, but there are reasons to keep
these in InstCombine instead as I learned with:
https://reviews.llvm.org/D28337


On Mon, Jan 16, 2017 at 9:11 AM, Sanjay Patel <spatel at rotateright.com>
wrote:

>
>
> On Mon, Jan 16, 2017 at 6:07 AM, Volkan Keles <vkeles at apple.com> wrote:
>
>>
>> On Jan 13, 2017, at 3:16 PM, Sanjay Patel <spatel at rotateright.com> wrote:
>>
>>
>>
>> On Fri, Jan 13, 2017 at 5:24 AM, Volkan Keles <vkeles at apple.com> wrote:
>>
>>>
>>> On Jan 12, 2017, at 4:35 PM, Sanjay Patel <spatel at rotateright.com>
>>> wrote:
>>>
>>> Hi Volkan,
>>>
>>> I reduced your test case and explained what I found below. It's not
>>> clear to me what you mean by linking the compile-time thread:
>>> 1. SCCP handles this case already, so we shouldn't need to add a
>>> duplicate fold to InstCombine and increase compile-time further?
>>>
>>> 2. You are opposed to the min/max canonicalization in this commit
>>> because it is increasing compile-time?
>>>
>>>
>>> Sorry, my e-mail was not clear. I meant the former, I wanted to get your
>>> opinion.
>>>
>>
>>
>> I understand the concern, but this commit (like the min/max commit -
>> https://reviews.llvm.org/D27531 - that was inspired by this thread and
>> referenced in the compile-time thread) is a very simple matcher. If these
>> kinds of commits are causing noticeable slowdowns, then we need a
>> structural fix...or LLVM is doing a lousy job of optimizing its own code -
>> PR28430 is a bit embarrassing.
>>
>> IMO, we need many more simple canonicalizations like this to make life
>> easier for the backend. I've explained the rationale for the min/max
>> additions (better codegen). This actually goes back to an earlier, more
>> general suggestion on llvm-dev that we should canonicalize harder towards
>> 'select' instructions in IR. We may want to rethink that strategy given the
>> compile-time creep, but I'm definitely not knowledgeable enough to make
>> that decision.
>>
>> Several suggestions were made in the compile-time thread about improving
>> instcombine, so I think that's where we should continue the discussion
>> about compile-time in general.
>>
>>
>> I’m not opposed to the min/max canonicalization and I’m not worried about
>> its impact on compile-time, it was related to InstCombine. I wanted to know
>> if that expensive call was going to be added to InstCombine.
>>
>>
> Sorry - I misunderstood your earlier answer. So you'd like to see this
> fold added to InstCombine because your pipeline doesn't include SCCP and
> you're not too concerned about compile-time. Can you file a bug report for
> this case, so we can reference it?
>
> I'm not sure how others will react. They might ask: "Why not run SCCP
> since compile-time isn't a problem?"
>
> FWIW, I started looking at InstCombine folds involving phi nodes with:
> https://reviews.llvm.org/D28625
> ...I haven't tried many loop optimizations before, so I'm still learning
> about safely handling those. The logic and pattern-matching to make the
> transform that you want for this fold is hopefully similar: we need to
> thread the phi's incoming values into the users and evaluate them.
>
>
>
>>
>> I am curious about something here though. You noticed this difference in
>> instsimplify behavior - does that mean that SCCP doesn't run in your
>> optimization pipeline?
>>
>>
>> No, it doesn’t.
>>
>>
>>
>>>
>>> As I said earlier, the canonicalization seems equally likely to enable
>>> more folding in InstSimplify as it does to bypass it. Do you agree?
>>>
>>>
>>> Yes, I agree.
>>>
>>>
>>> My motivation for canonicalization of min/max patterns is that the
>>> backend is full of holes recognizing and lowering these patterns, so the
>>> more we can standardize them here in IR, the less we have to worry about
>>> botching the lowering. I have not attempted to weigh the compile-time cost
>>> of canonicalization against the potential runtime perf wins.
>>>
>>>
>>>
>> Volkan
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170116/fff36e87/attachment.html>