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

[Sanjay Patel via llvm-commits]

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/e0a812f8/attachment.html>