Re: [PATCH] Fix up sqrt(x) < c and sqrt(x) >= c match.pd folding (PR tree-optimization/91734, take 2)

[Jeff Law <law@redhat.com>]

On 9/21/19 12:14 AM, Jakub Jelinek wrote:
> On Mon, Sep 16, 2019 at 08:56:58AM +0200, Richard Biener wrote:
>>> As mentioned in the PR, the sqrt (x) < c optimization into x < c*c
>>> sometimes breaks the boundary case, if c2=c*c is inexact then in some cases
>>> we need to optimize it into x <= c*c rather than x < c*c.  The original
>>> bugreport is when c is small and c2 is 0.0, then obviously we need <= 0.0
>>> rather than < 0.0, but the testcase includes another example where it makes
>>> a difference, plus has a >= testcase too.
>>>
>>> Bootstrapped/regtested on powerpc64le-linux, ok for trunk?
>>
>> I was hoping Joseph might chime in here...  anyway, does this assume
>> round-to-nearest or does it work with round to +-Inf as well?  I
>> realize this all is under flag_unsafe_math_optimizations, but
>> this flag is notoriously underspecified...  So the question is
>> whether we should disable the transform if c*c isn't exact and
>> flag_rounding_math?  The transform also doesn't seem to guard
>> against isnan (c) (-funsafe-math-optimizations sets
>> -fno-trapping-math and -fno-signed-zeros but not -ffinite-math-only
>> or disables itself on -frounding-math)
> 
> Here is an updated patch, which on top of the previous patch:
> 1) punts for -frounding-math
> 2) punts for sqrt comparisons against NaN constant
> 3) for the c*c inexact also handles the other two comparisons that
> apparently need to be handled too
> 4) for all 4 comparisons also checks nexttoward (c2, 0.0) or nexttoward (c2,
> inf) depending on the comparison kind, because as Joseph correctly noted,
> with rounding to nearest up to 3 different floating point values can have
> the same sqrt result, and if c2 is the middle one from them, we need to use
> the 1 ulp smaller or larger one in the comparison
> 5) had to adjust the testcase, because while it worked fine on powerpc64le,
> on x86_64 if the test is linked with -ffast-math/-Ofast etc., crtfastmath.o
> is linked in and subnormals are flushed to zero, which is not what we want
> for the testcase (at least for a subset of the tests).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> BTW, I've used attached programs to look for the problematic cases on random
> float/doubles and the cases the patch handles seem to be the only
> problematic ones, there is never need to go further than one nexttoward to 0
> or inf.
> 
> 2019-09-21  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/91734
> 	* generic-match-head.c: Include fold-const-call.h.
> 	* match.pd (sqrt(x) cmp c): Check the boundary value and
> 	in case inexact computation of c*c affects comparison of the boundary,
> 	turn LT_EXPR into LE_EXPR, GE_EXPR into GT_EXPR, LE_EXPR into LT_EXPR
> 	or GT_EXPR into GE_EXPR.  Punt for sqrt comparisons against NaN and
> 	for -frounding-math.  For c2, try the next smaller or larger floating
> 	point constant depending on comparison code and if it has the same
> 	sqrt as c2, use it instead of c2.
> 
> 	* gcc.dg/pr91734.c: New test.
OK.  One might argue that some of this code needs to be refactored into
functions to make visual parsing simpler.  But I'm not going to insist
on it right now.

jeff