match.pd patch: min(-x, -y), min(~x, ~y)

Hello,

another simple transformation.

Instead of the ":s", I had single_use (@2) || single_use (@3), but changed 
it for simplicity. There may be some patterns in match.pd where we want 
something like that though, as requiring single_use on many expressions 
may be stricter than we need.

We could generalize to cases where overflow is not undefined if we know 
(VRP) that the variables are not TYPE_MIN_VALUE, but that didn't look like 
a priority.

Bootstrap+regtest on powerpc64le-unknown-linux-gnu.

2016-04-21  Marc Glisse  <marc.glisse@inria.fr>

gcc/
 	* match.pd (min(-x, -y), max(-x, -y), min(~x, ~y), max(~x, ~y)):
 	New transformations.

gcc/testsuite/
 	* gcc.dg/tree-ssa/minmax-2.c: New testcase.

On Thu, Apr 21, 2016 at 12:32 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> Hello,
>
> another simple transformation.
>
> Instead of the ":s", I had single_use (@2) || single_use (@3), but changed
> it for simplicity. There may be some patterns in match.pd where we want
> something like that though, as requiring single_use on many expressions may
> be stricter than we need.
>
> We could generalize to cases where overflow is not undefined if we know
> (VRP) that the variables are not TYPE_MIN_VALUE, but that didn't look like a
> priority.
>
> Bootstrap+regtest on powerpc64le-unknown-linux-gnu.

Ok.  I thought about using negate_expr_p but min(-x,5) -> -max(x, -5) doesn't
look like an obvious win.

Thanks,
Richard.

> 2016-04-21  Marc Glisse  <marc.glisse@inria.fr>
>
> gcc/
>         * match.pd (min(-x, -y), max(-x, -y), min(~x, ~y), max(~x, ~y)):
>         New transformations.
>
> gcc/testsuite/
>         * gcc.dg/tree-ssa/minmax-2.c: New testcase.
>
>
> --
> Marc Glisse
> Index: gcc/match.pd
> ===================================================================
> --- gcc/match.pd        (revision 235292)
> +++ gcc/match.pd        (working copy)
> @@ -1215,20 +1215,36 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>     MIN and MAX don't honor that, so only transform if -ffinite-math-only
>     is set.  C99 doesn't require -0.0 to be handled, so we don't have to
>     worry about it either.  */
>  (if (flag_finite_math_only)
>   (simplify
>    (FMIN @0 @1)
>    (min @0 @1))
>   (simplify
>    (FMAX @0 @1)
>    (max @0 @1)))
> +/* min (-A, -B) -> -max (A, B)  */
> +(for minmax (min max FMIN FMAX)
> +     maxmin (max min FMAX FMIN)
> + (simplify
> +  (minmax (negate:s@2 @0) (negate:s@3 @1))
> +  (if (FLOAT_TYPE_P (TREE_TYPE (@0))
> +       || (ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0))
> +           && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (@0))))
> +   (negate (maxmin @0 @1)))))
> +/* MIN (~X, ~Y) -> ~MAX (X, Y)
> +   MAX (~X, ~Y) -> ~MIN (X, Y)  */
> +(for minmax (min max)
> + maxmin (max min)
> + (simplify
> +  (minmax (bit_not:s@2 @0) (bit_not:s@3 @1))
> +  (bit_not (maxmin @0 @1))))
>
>  /* Simplifications of shift and rotates.  */
>
>  (for rotate (lrotate rrotate)
>   (simplify
>    (rotate integer_all_onesp@0 @1)
>    @0))
>
>  /* Optimize -1 >> x for arithmetic right shifts.  */
>  (simplify
> Index: gcc/testsuite/gcc.dg/tree-ssa/minmax-2.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/tree-ssa/minmax-2.c    (revision 0)
> +++ gcc/testsuite/gcc.dg/tree-ssa/minmax-2.c    (working copy)
> @@ -0,0 +1,10 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -fstrict-overflow -fdump-tree-optimized" } */
> +
> +static int max(int a,int b){return (a<b)?b:a;}
> +int f(int x,int y){return max(-x,-y);}
> +int g(int x,int y){return max(~x,~y);}
> +double h(double x,double y){return __builtin_fmax(-x,-y);}
> +
> +/* { dg-final { scan-tree-dump-times "MIN_EXPR" 2 "optimized" } } */
> +/* { dg-final { scan-tree-dump "__builtin_fmin" "optimized" } } */
>

Hi Marc,

On 21/04/16 11:32, Marc Glisse wrote:
> Hello,
>
> another simple transformation.
>
> Instead of the ":s", I had single_use (@2) || single_use (@3), but changed it for simplicity. There may be some patterns in match.pd where we want something like that though, as requiring single_use on many expressions may be stricter 
> than we need.
>
> We could generalize to cases where overflow is not undefined if we know (VRP) that the variables are not TYPE_MIN_VALUE, but that didn't look like a priority.
>
> Bootstrap+regtest on powerpc64le-unknown-linux-gnu.
>
> 2016-04-21  Marc Glisse  <marc.glisse@inria.fr>
>
> gcc/
>     * match.pd (min(-x, -y), max(-x, -y), min(~x, ~y), max(~x, ~y)):
>     New transformations.
>
> gcc/testsuite/
>     * gcc.dg/tree-ssa/minmax-2.c: New testcase.
>
>

I see the new testcase failing on aarch64:
FAIL: gcc.dg/tree-ssa/minmax-2.c scan-tree-dump optimized "__builtin_fmin"

The tree dump for the function 'h' is:
h (double x, double y)
{
   double _2;
   double _4;
   double _5;

   <bb 2>:
   _2 = -x_1(D);
   _4 = -y_3(D);
   _5 = __builtin_fmax (_2, _4);
   return _5;

}

Kyrill

On Fri, 22 Apr 2016, Kyrill Tkachov wrote:

>> 2016-04-21  Marc Glisse  <marc.glisse@inria.fr>
>> 
>> gcc/
>>     * match.pd (min(-x, -y), max(-x, -y), min(~x, ~y), max(~x, ~y)):
>>     New transformations.
>> 
>> gcc/testsuite/
>>     * gcc.dg/tree-ssa/minmax-2.c: New testcase.
>> 
>> 
>
> I see the new testcase failing on aarch64:
> FAIL: gcc.dg/tree-ssa/minmax-2.c scan-tree-dump optimized "__builtin_fmin"

Strange, it seems to work in 
https://gcc.gnu.org/ml/gcc-testresults/2016-04/msg02120.html

Is that on some freestanding kind of setup where the builtin might be 
disabled?

On 22/04/16 10:42, Marc Glisse wrote:
> On Fri, 22 Apr 2016, Kyrill Tkachov wrote:
>
>>> 2016-04-21  Marc Glisse <marc.glisse@inria.fr>
>>>
>>> gcc/
>>>     * match.pd (min(-x, -y), max(-x, -y), min(~x, ~y), max(~x, ~y)):
>>>     New transformations.
>>>
>>> gcc/testsuite/
>>>     * gcc.dg/tree-ssa/minmax-2.c: New testcase.
>>>
>>>
>>
>> I see the new testcase failing on aarch64:
>> FAIL: gcc.dg/tree-ssa/minmax-2.c scan-tree-dump optimized "__builtin_fmin"
>
> Strange, it seems to work in https://gcc.gnu.org/ml/gcc-testresults/2016-04/msg02120.html
>
> Is that on some freestanding kind of setup where the builtin might be disabled?
>

Ah, this is aarch64-none-elf which uses newlib as the C library.
Let me check on aarch64-none-linux-gnu and get back to you.

Thanks,
Kyrill

On 22/04/16 10:43, Kyrill Tkachov wrote:
>
> On 22/04/16 10:42, Marc Glisse wrote:
>> On Fri, 22 Apr 2016, Kyrill Tkachov wrote:
>>
>>>> 2016-04-21  Marc Glisse <marc.glisse@inria.fr>
>>>>
>>>> gcc/
>>>>     * match.pd (min(-x, -y), max(-x, -y), min(~x, ~y), max(~x, ~y)):
>>>>     New transformations.
>>>>
>>>> gcc/testsuite/
>>>>     * gcc.dg/tree-ssa/minmax-2.c: New testcase.
>>>>
>>>>
>>>
>>> I see the new testcase failing on aarch64:
>>> FAIL: gcc.dg/tree-ssa/minmax-2.c scan-tree-dump optimized "__builtin_fmin"
>>
>> Strange, it seems to work in https://gcc.gnu.org/ml/gcc-testresults/2016-04/msg02120.html
>>
>> Is that on some freestanding kind of setup where the builtin might be disabled?
>>
>
> Ah, this is aarch64-none-elf which uses newlib as the C library.
> Let me check on aarch64-none-linux-gnu and get back to you.
>

Yeah, I see it passing on aarch64-none-linux-gnu.
Do we have an appropriate effective target check to gate this test on?

Kyrill

> Thanks,
> Kyrill
>

On Fri, 22 Apr 2016, Kyrill Tkachov wrote:

>
> On 22/04/16 10:43, Kyrill Tkachov wrote:
>> 
>> On 22/04/16 10:42, Marc Glisse wrote:
>>> On Fri, 22 Apr 2016, Kyrill Tkachov wrote:
>>> 
>>>>> 2016-04-21  Marc Glisse <marc.glisse@inria.fr>
>>>>> 
>>>>> gcc/
>>>>>     * match.pd (min(-x, -y), max(-x, -y), min(~x, ~y), max(~x, ~y)):
>>>>>     New transformations.
>>>>> 
>>>>> gcc/testsuite/
>>>>>     * gcc.dg/tree-ssa/minmax-2.c: New testcase.
>>>>> 
>>>>> 
>>>> 
>>>> I see the new testcase failing on aarch64:
>>>> FAIL: gcc.dg/tree-ssa/minmax-2.c scan-tree-dump optimized 
>>>> "__builtin_fmin"
>>> 
>>> Strange, it seems to work in 
>>> https://gcc.gnu.org/ml/gcc-testresults/2016-04/msg02120.html
>>> 
>>> Is that on some freestanding kind of setup where the builtin might be 
>>> disabled?
>>> 
>> 
>> Ah, this is aarch64-none-elf which uses newlib as the C library.
>> Let me check on aarch64-none-linux-gnu and get back to you.
>> 
>
> Yeah, I see it passing on aarch64-none-linux-gnu.
> Do we have an appropriate effective target check to gate this test on?

I don't know, I have a hard time finding something related. I am not even 
convinced the test should be skipped. It looks like __builtin_fmax was 
recognized, otherwise you would get a warning and a conversion int-double. 
Maybe gimple_call_combined_fn rejects it? Ah, builtins.def declares it 
with DEF_C99_BUILTIN, which checks targetm.libc_has_function 
(function_c99_misc). I assume newlib fails that check? That would make 
c99_runtime a relevant target check.

On 22/04/16 11:34, Marc Glisse wrote:
> On Fri, 22 Apr 2016, Kyrill Tkachov wrote:
>
>>
>> On 22/04/16 10:43, Kyrill Tkachov wrote:
>>>
>>> On 22/04/16 10:42, Marc Glisse wrote:
>>>> On Fri, 22 Apr 2016, Kyrill Tkachov wrote:
>>>>
>>>>>> 2016-04-21  Marc Glisse <marc.glisse@inria.fr>
>>>>>>
>>>>>> gcc/
>>>>>>     * match.pd (min(-x, -y), max(-x, -y), min(~x, ~y), max(~x, ~y)):
>>>>>>     New transformations.
>>>>>>
>>>>>> gcc/testsuite/
>>>>>>     * gcc.dg/tree-ssa/minmax-2.c: New testcase.
>>>>>>
>>>>>>
>>>>>
>>>>> I see the new testcase failing on aarch64:
>>>>> FAIL: gcc.dg/tree-ssa/minmax-2.c scan-tree-dump optimized "__builtin_fmin"
>>>>
>>>> Strange, it seems to work in https://gcc.gnu.org/ml/gcc-testresults/2016-04/msg02120.html
>>>>
>>>> Is that on some freestanding kind of setup where the builtin might be disabled?
>>>>
>>>
>>> Ah, this is aarch64-none-elf which uses newlib as the C library.
>>> Let me check on aarch64-none-linux-gnu and get back to you.
>>>
>>
>> Yeah, I see it passing on aarch64-none-linux-gnu.
>> Do we have an appropriate effective target check to gate this test on?
>
> I don't know, I have a hard time finding something related. I am not even convinced the test should be skipped. It looks like __builtin_fmax was recognized, otherwise you would get a warning and a conversion int-double. Maybe 
> gimple_call_combined_fn rejects it? Ah, builtins.def declares it with DEF_C99_BUILTIN, which checks targetm.libc_has_function (function_c99_misc). I assume newlib fails that check? That would make c99_runtime a relevant target check.
>

Yeah, adding the below makes this test UNSUPPORTED on aarch64-none-elf.
/* { dg-add-options c99_runtime } */
/* { dg-require-effective-target c99_runtime } */

I'll prepare a patch.

Thanks,
Kyrill