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

[Sanjay Patel via llvm-commits]

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. :)

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).

On Fri, Jan 27, 2017 at 4:43 PM, Davide Italiano <davide at freebsd.org> wrote:

> On Fri, Jan 27, 2017 at 3:26 PM, Sanjay Patel via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
> > Author: spatel
> > Date: Fri Jan 27 17:26:27 2017
> > New Revision: 293345
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=293345&view=rev
> > Log:
> > [InstCombine] move icmp transforms that might be recognized as min/max
> and inf-loop (PR31751)
> >
> > This is a minimal patch to avoid the infinite loop in:
> > https://llvm.org/bugs/show_bug.cgi?id=31751
> >
> > But the general problem is bigger: we're not canonicalizing all of the
> min/max forms reported
> > by value tracking's matchSelectPattern(), and we don't define min/max
> consistently. Some code
> > uses matchSelectPattern(), other code uses matchers like m_Umax, and
> others have their own
> > inline definitions which may be subtly different from any of the above.
> >
> > The reason that the test cases in this patch need a cast op to trigger
> is because we don't
> > (yet) canonicalize all min/max forms based on matchSelectPattern() in
> > canonicalizeMinMaxWithConstant(), but we do make min/max+cast
> transforms based on
> > matchSelectPattern() in visitSelectInst().
> >
> > The location of the icmp transforms that trigger the inf-loop seems
> arbitrary at best, so
> > I'm moving those behind the min/max fence in visitICmpInst() as the
> quick fix.
> >
>
> Ehm.. this workaround seems a little fragile. Maybe reverting the
> original commit which introduced this behavior would be a better thing
> for now? https://reviews.llvm.org/rL286318 (i.e. until we have an idea
> on how to fix this properly)
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170127/1b97d986/attachment.html>