[llvm] r293345 - [InstCombine] move icmp transforms that might be recognized as min/max and inf-loop (PR31751)

[Davide Italiano via llvm-commits]

On Fri, Jan 27, 2017 at 4:42 PM, Sanjay Patel <spatel at rotateright.com> wrote:
> This patch targets precisely the patterns that were matched in r286318, so I
> don't see how reverting that would be a better solution. And I was being
> generous by calling the existing code "arbitrary" - those folds were in
> foldICmpUsingKnownBits(), and they have nothing do with known bits. :)
>

This problem seems to manifest only after you added the matchers, right?
So maybe a different solution would be that not matching these
constructs until there are problems surrounding them (as we don't
really know what could be the effect down the road). If you don't want
to do that in trunk and see if things stick together, fair enough, but
I think it's more cautious removing the matchers for 4.0 as they may
expose other problems we'
re not really aware of. Up to you anyway.

> We'd still have the general problem that various parts of the code define
> min/max differently. The bigger solution is to canonicalize min/max harder
> and standardize the min/max definitions, but that's going to take time to
> sort out all the places we have ad hoc min/max checks and then make sure
> that the code still does what was intended (and doesn't induce inf-loop from
> some other transform).
>

Agree. I think this problem should be addressed before adding new
matchers for min/max and not in parallel (or after).

Thanks,

--
Davide