* [PATCH GCC][1/4]Simplify (convert1 (minmax ((convert2 (x) c)))) into minmax (x c)
@ 2016-10-25 11:21 Bin Cheng
2016-10-25 11:48 ` Richard Biener
0 siblings, 1 reply; 9+ messages in thread
From: Bin Cheng @ 2016-10-25 11:21 UTC (permalink / raw)
To: gcc-patches; +Cc: nd
[-- Attachment #1: Type: text/plain, Size: 1101 bytes --]
Hi,
This is a patch set adding various match.pd patterns in order to generate simplified MIN/MAX_EXPR mostly from COND_EXPR. This is the first one optimizing (convert1 (minmax ((convert2 (x) c)))) to minmax (x c), if convert2 promotes x and convert1 demotes back to x's type. With this patch, generated assembly for test:
.L4:
ldr q0, [x3, x1]
add w2, w2, 1
cmp w0, w2
ushll v2.4s, v0.4h, 0
ushll2 v1.4s, v0.8h, 0
umin v2.4s, v2.4s, v3.4s
umin v1.4s, v1.4s, v3.4s
xtn v4.4h, v2.4s
xtn2 v4.8h, v1.4s
str q4, [x3, x1]
add x1, x1, 16
bhi .L4
can be improved to:
.L4:
ldr q0, [x3, x1]
add w2, w2, 1
cmp w0, w2
umin v1.8h, v0.8h, v2.8h
str q1, [x3, x1]
add x1, x1, 16
bhi .L4
Bootstrap and test on x86_64 and AArch64 for whole patch set. Is it OK?
Thanks,
bin
2016-10-21 Bin Cheng <bin.cheng@arm.com>
* match.pd ((convert1 (minmax ((convert2 (x) c)))) -> minmax (x c)):
New pattern.
gcc/testsuite/ChangeLog
2016-10-21 Bin Cheng <bin.cheng@arm.com>
* gcc.dg/fold-convmaxconv-1.c: New test.
* gcc.dg/fold-convminconv-1.c: New test.
[-- Attachment #2: 01-simplify-convminmaxconv-20161020.txt --]
[-- Type: text/plain, Size: 2311 bytes --]
diff --git a/gcc/match.pd b/gcc/match.pd
index b782a1e..7365bc1 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -1331,6 +1331,28 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
&& TYPE_MIN_VALUE (type)
&& operand_equal_p (@1, TYPE_MIN_VALUE (type), OEP_ONLY_CONST))
@0)))
+
+#if GIMPLE
+/* (convert1 (minmax ((convert2 (x) c)))) -> minmax (x c) if convert2
+ promotes x and convert1 demotes back to x's type. */
+
+(for minmax (min max)
+ (simplify
+ (convert (minmax@0 (convert @1) INTEGER_CST@2))
+ (if (types_compatible_p (TREE_TYPE (@1), type))
+ (with
+ {
+ tree minmax_type = TREE_TYPE (@0);
+ signop sgn = TYPE_SIGN (type);
+ widest_int type_min = widest_int::from (wi::min_value (type), sgn);
+ widest_int type_max = widest_int::from (wi::max_value (type), sgn);
+ }
+ (if (sgn == TYPE_SIGN (minmax_type)
+ && TYPE_PRECISION (minmax_type) >= TYPE_PRECISION (type)
+ && wi::to_widest (@2) >= type_min && wi::to_widest (@2) <= type_max)
+ (minmax @1 { fold_convert (type, @2); }))))))
+#endif
+
(for minmax (FMIN FMAX)
/* If either argument is NaN, return the other one. Avoid the
transformation if we get (and honor) a signalling NaN. */
diff --git a/gcc/testsuite/gcc.dg/fold-convmaxconv-1.c b/gcc/testsuite/gcc.dg/fold-convmaxconv-1.c
new file mode 100644
index 0000000..3ffff8b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fold-convmaxconv-1.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-optimized" } */
+
+int foo (short a[], int x)
+{
+ unsigned int i;
+ for (i = 0; i < 1000; i++)
+ {
+ x = a[i];
+ a[i] = (x <= 0 ? 0 : x);
+ }
+ return x;
+}
+
+/* { dg-final { scan-tree-dump-not " = MAX_EXPR <x_\[0-9\]*" "optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/fold-convminconv-1.c b/gcc/testsuite/gcc.dg/fold-convminconv-1.c
new file mode 100644
index 0000000..f4a048e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fold-convminconv-1.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-optimized" } */
+
+int foo (unsigned short a[], unsigned int x)
+{
+ unsigned int i;
+ for (i = 0; i < 1000; i++)
+ {
+ x = a[i];
+ a[i] = (x >= 255 ? 255 : x);
+ }
+ return x;
+}
+
+/* { dg-final { scan-tree-dump-not " = MIN_EXPR <x_\[0-9\]*" "optimized" } } */
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH GCC][1/4]Simplify (convert1 (minmax ((convert2 (x) c)))) into minmax (x c)
2016-10-25 11:21 [PATCH GCC][1/4]Simplify (convert1 (minmax ((convert2 (x) c)))) into minmax (x c) Bin Cheng
@ 2016-10-25 11:48 ` Richard Biener
2016-10-26 13:51 ` Bin.Cheng
0 siblings, 1 reply; 9+ messages in thread
From: Richard Biener @ 2016-10-25 11:48 UTC (permalink / raw)
To: Bin Cheng; +Cc: gcc-patches, nd
On Tue, Oct 25, 2016 at 1:21 PM, Bin Cheng <Bin.Cheng@arm.com> wrote:
> Hi,
> This is a patch set adding various match.pd patterns in order to generate simplified MIN/MAX_EXPR mostly from COND_EXPR. This is the first one optimizing (convert1 (minmax ((convert2 (x) c)))) to minmax (x c), if convert2 promotes x and convert1 demotes back to x's type. With this patch, generated assembly for test:
> .L4:
> ldr q0, [x3, x1]
> add w2, w2, 1
> cmp w0, w2
> ushll v2.4s, v0.4h, 0
> ushll2 v1.4s, v0.8h, 0
> umin v2.4s, v2.4s, v3.4s
> umin v1.4s, v1.4s, v3.4s
> xtn v4.4h, v2.4s
> xtn2 v4.8h, v1.4s
> str q4, [x3, x1]
> add x1, x1, 16
> bhi .L4
>
> can be improved to:
> .L4:
> ldr q0, [x3, x1]
> add w2, w2, 1
> cmp w0, w2
> umin v1.8h, v0.8h, v2.8h
> str q1, [x3, x1]
> add x1, x1, 16
> bhi .L4
>
> Bootstrap and test on x86_64 and AArch64 for whole patch set. Is it OK?
Why restrict to GIMPLE?
+/* (convert1 (minmax ((convert2 (x) c)))) -> minmax (x c) if convert2
+ promotes x and convert1 demotes back to x's type. */
+
+(for minmax (min max)
+ (simplify
+ (convert (minmax@0 (convert @1) INTEGER_CST@2))
+ (if (types_compatible_p (TREE_TYPE (@1), type))
comment mentions convert1 and convert2, just convert is correct I think.
Please use types_match instead of types_compatible_p, this is a
wrapper that does the correct thing for both GENERIC and GIMPLE.
+ (with
+ {
+ tree minmax_type = TREE_TYPE (@0);
+ signop sgn = TYPE_SIGN (type);
+ widest_int type_min = widest_int::from (wi::min_value (type), sgn);
+ widest_int type_max = widest_int::from (wi::max_value (type), sgn);
+ }
+ (if (sgn == TYPE_SIGN (minmax_type)
+ && TYPE_PRECISION (minmax_type) >= TYPE_PRECISION (type)
+ && wi::to_widest (@2) >= type_min && wi::to_widest (@2) <= type_max)
instead of this you can use int_fits_type_p (type, @2)
+ (minmax @1 { fold_convert (type, @2); }))))))
use
(minmax @1 (convert @2))
I believe the transform is also a win if @2 is not a constant but similarly
promoted as @1. This slightly complicates the patter and thus can
be done as followup (if we think it's useful at this point).
With the simplification you should get rid of the with{}
Thanks,
Richard.
> Thanks,
> bin
>
> 2016-10-21 Bin Cheng <bin.cheng@arm.com>
>
> * match.pd ((convert1 (minmax ((convert2 (x) c)))) -> minmax (x c)):
> New pattern.
>
> gcc/testsuite/ChangeLog
> 2016-10-21 Bin Cheng <bin.cheng@arm.com>
>
> * gcc.dg/fold-convmaxconv-1.c: New test.
> * gcc.dg/fold-convminconv-1.c: New test.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH GCC][1/4]Simplify (convert1 (minmax ((convert2 (x) c)))) into minmax (x c)
2016-10-25 11:48 ` Richard Biener
@ 2016-10-26 13:51 ` Bin.Cheng
2016-10-26 14:04 ` Marc Glisse
0 siblings, 1 reply; 9+ messages in thread
From: Bin.Cheng @ 2016-10-26 13:51 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2764 bytes --]
On Tue, Oct 25, 2016 at 12:48 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Tue, Oct 25, 2016 at 1:21 PM, Bin Cheng <Bin.Cheng@arm.com> wrote:
>> Hi,
>> This is a patch set adding various match.pd patterns in order to generate simplified MIN/MAX_EXPR mostly from COND_EXPR. This is the first one optimizing (convert1 (minmax ((convert2 (x) c)))) to minmax (x c), if convert2 promotes x and convert1 demotes back to x's type. With this patch, generated assembly for test:
>> .L4:
>> ldr q0, [x3, x1]
>> add w2, w2, 1
>> cmp w0, w2
>> ushll v2.4s, v0.4h, 0
>> ushll2 v1.4s, v0.8h, 0
>> umin v2.4s, v2.4s, v3.4s
>> umin v1.4s, v1.4s, v3.4s
>> xtn v4.4h, v2.4s
>> xtn2 v4.8h, v1.4s
>> str q4, [x3, x1]
>> add x1, x1, 16
>> bhi .L4
>>
>> can be improved to:
>> .L4:
>> ldr q0, [x3, x1]
>> add w2, w2, 1
>> cmp w0, w2
>> umin v1.8h, v0.8h, v2.8h
>> str q1, [x3, x1]
>> add x1, x1, 16
>> bhi .L4
>>
>> Bootstrap and test on x86_64 and AArch64 for whole patch set. Is it OK?
>
> Why restrict to GIMPLE?
>
> +/* (convert1 (minmax ((convert2 (x) c)))) -> minmax (x c) if convert2
> + promotes x and convert1 demotes back to x's type. */
> +
> +(for minmax (min max)
> + (simplify
> + (convert (minmax@0 (convert @1) INTEGER_CST@2))
> + (if (types_compatible_p (TREE_TYPE (@1), type))
>
> comment mentions convert1 and convert2, just convert is correct I think.
>
> Please use types_match instead of types_compatible_p, this is a
> wrapper that does the correct thing for both GENERIC and GIMPLE.
>
> + (with
> + {
> + tree minmax_type = TREE_TYPE (@0);
> + signop sgn = TYPE_SIGN (type);
> + widest_int type_min = widest_int::from (wi::min_value (type), sgn);
> + widest_int type_max = widest_int::from (wi::max_value (type), sgn);
> + }
> + (if (sgn == TYPE_SIGN (minmax_type)
> + && TYPE_PRECISION (minmax_type) >= TYPE_PRECISION (type)
> + && wi::to_widest (@2) >= type_min && wi::to_widest (@2) <= type_max)
>
> instead of this you can use int_fits_type_p (type, @2)
>
> + (minmax @1 { fold_convert (type, @2); }))))))
>
> use
>
> (minmax @1 (convert @2))
>
> I believe the transform is also a win if @2 is not a constant but similarly
> promoted as @1. This slightly complicates the patter and thus can
> be done as followup (if we think it's useful at this point).
>
> With the simplification you should get rid of the with{}
Thanks for reviewing, updated patch attached. Is it OK?
Thanks,
bin
[-- Attachment #2: 01-simplify-convminmaxconv-20161023.txt --]
[-- Type: text/plain, Size: 1946 bytes --]
diff --git a/gcc/match.pd b/gcc/match.pd
index 767d23a..086709f 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -1337,6 +1337,16 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
&& TYPE_MIN_VALUE (type)
&& operand_equal_p (@1, TYPE_MIN_VALUE (type), OEP_ONLY_CONST))
@0)))
+
+/* (convert (minmax ((convert (x) c)))) -> minmax (x c) if x is promoted
+ and the outer convert demotes the expression back to x's type. */
+(for minmax (min max)
+ (simplify
+ (convert (minmax@0 (convert @1) INTEGER_CST@2))
+ (if (types_match (@1, type) && int_fits_type_p (@2, type)
+ && TYPE_PRECISION (TREE_TYPE (@0)) > TYPE_PRECISION (TREE_TYPE (@1)))
+ (minmax @1 (convert @2)))))
+
(for minmax (FMIN FMAX)
/* If either argument is NaN, return the other one. Avoid the
transformation if we get (and honor) a signalling NaN. */
diff --git a/gcc/testsuite/gcc.dg/fold-convmaxconv-1.c b/gcc/testsuite/gcc.dg/fold-convmaxconv-1.c
new file mode 100644
index 0000000..3ffff8b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fold-convmaxconv-1.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-optimized" } */
+
+int foo (short a[], int x)
+{
+ unsigned int i;
+ for (i = 0; i < 1000; i++)
+ {
+ x = a[i];
+ a[i] = (x <= 0 ? 0 : x);
+ }
+ return x;
+}
+
+/* { dg-final { scan-tree-dump-not " = MAX_EXPR <x_\[0-9\]*" "optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/fold-convminconv-1.c b/gcc/testsuite/gcc.dg/fold-convminconv-1.c
new file mode 100644
index 0000000..f4a048e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fold-convminconv-1.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-optimized" } */
+
+int foo (unsigned short a[], unsigned int x)
+{
+ unsigned int i;
+ for (i = 0; i < 1000; i++)
+ {
+ x = a[i];
+ a[i] = (x >= 255 ? 255 : x);
+ }
+ return x;
+}
+
+/* { dg-final { scan-tree-dump-not " = MIN_EXPR <x_\[0-9\]*" "optimized" } } */
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH GCC][1/4]Simplify (convert1 (minmax ((convert2 (x) c)))) into minmax (x c)
2016-10-26 13:51 ` Bin.Cheng
@ 2016-10-26 14:04 ` Marc Glisse
2016-10-26 14:10 ` Bin.Cheng
0 siblings, 1 reply; 9+ messages in thread
From: Marc Glisse @ 2016-10-26 14:04 UTC (permalink / raw)
To: Bin.Cheng; +Cc: Richard Biener, gcc-patches
On Wed, 26 Oct 2016, Bin.Cheng wrote:
> Thanks for reviewing, updated patch attached. Is it OK?
+/* (convert (minmax ((convert (x) c)))) -> minmax (x c) if x is promoted
+ and the outer convert demotes the expression back to x's type. */
+(for minmax (min max)
+ (simplify
+ (convert (minmax@0 (convert @1) INTEGER_CST@2))
+ (if (types_match (@1, type) && int_fits_type_p (@2, type)
+ && TYPE_PRECISION (TREE_TYPE (@0)) > TYPE_PRECISION (TREE_TYPE (@1)))
+ (minmax @1 (convert @2)))))
Don't you have a problem if @1 is signed but not @0?
(int)max((unsigned long)(-2),3ul)
seems to satisfy your conditions, but is not the same as
max(-2,3)
--
Marc Glisse
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH GCC][1/4]Simplify (convert1 (minmax ((convert2 (x) c)))) into minmax (x c)
2016-10-26 14:04 ` Marc Glisse
@ 2016-10-26 14:10 ` Bin.Cheng
2016-10-26 14:31 ` Bin.Cheng
0 siblings, 1 reply; 9+ messages in thread
From: Bin.Cheng @ 2016-10-26 14:10 UTC (permalink / raw)
To: gcc-patches List; +Cc: Richard Biener
On Wed, Oct 26, 2016 at 3:04 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Wed, 26 Oct 2016, Bin.Cheng wrote:
>
>> Thanks for reviewing, updated patch attached. Is it OK?
>
>
> +/* (convert (minmax ((convert (x) c)))) -> minmax (x c) if x is promoted
> + and the outer convert demotes the expression back to x's type. */
> +(for minmax (min max)
> + (simplify
> + (convert (minmax@0 (convert @1) INTEGER_CST@2))
> + (if (types_match (@1, type) && int_fits_type_p (@2, type)
> + && TYPE_PRECISION (TREE_TYPE (@0)) > TYPE_PRECISION (TREE_TYPE
> (@1)))
> + (minmax @1 (convert @2)))))
>
> Don't you have a problem if @1 is signed but not @0?
> (int)max((unsigned long)(-2),3ul)
> seems to satisfy your conditions, but is not the same as
> max(-2,3)
Ah, yes. I falsely removed sign check from the original patch. Will
update that.
Thanks,
bin
>
> --
> Marc Glisse
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH GCC][1/4]Simplify (convert1 (minmax ((convert2 (x) c)))) into minmax (x c)
2016-10-26 14:10 ` Bin.Cheng
@ 2016-10-26 14:31 ` Bin.Cheng
2016-10-26 15:05 ` Marc Glisse
0 siblings, 1 reply; 9+ messages in thread
From: Bin.Cheng @ 2016-10-26 14:31 UTC (permalink / raw)
To: gcc-patches List; +Cc: Richard Biener, Marc Glisse
[-- Attachment #1: Type: text/plain, Size: 998 bytes --]
On Wed, Oct 26, 2016 at 3:10 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
> On Wed, Oct 26, 2016 at 3:04 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
>> On Wed, 26 Oct 2016, Bin.Cheng wrote:
>>
>>> Thanks for reviewing, updated patch attached. Is it OK?
>>
>>
>> +/* (convert (minmax ((convert (x) c)))) -> minmax (x c) if x is promoted
>> + and the outer convert demotes the expression back to x's type. */
>> +(for minmax (min max)
>> + (simplify
>> + (convert (minmax@0 (convert @1) INTEGER_CST@2))
>> + (if (types_match (@1, type) && int_fits_type_p (@2, type)
>> + && TYPE_PRECISION (TREE_TYPE (@0)) > TYPE_PRECISION (TREE_TYPE
>> (@1)))
>> + (minmax @1 (convert @2)))))
>>
>> Don't you have a problem if @1 is signed but not @0?
>> (int)max((unsigned long)(-2),3ul)
>> seems to satisfy your conditions, but is not the same as
>> max(-2,3)
> Ah, yes. I falsely removed sign check from the original patch. Will
> update that.
>
Here it is. Sorry for the mistake.
Thanks,
bin
[-- Attachment #2: 01-simplify-convminmaxconv-20161024.txt --]
[-- Type: text/plain, Size: 1994 bytes --]
diff --git a/gcc/match.pd b/gcc/match.pd
index 767d23a..73bee34 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -1337,6 +1337,17 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
&& TYPE_MIN_VALUE (type)
&& operand_equal_p (@1, TYPE_MIN_VALUE (type), OEP_ONLY_CONST))
@0)))
+
+/* (convert (minmax ((convert (x) c)))) -> minmax (x c) if x is promoted
+ and the outer convert demotes the expression back to x's type. */
+(for minmax (min max)
+ (simplify
+ (convert (minmax@0 (convert @1) INTEGER_CST@2))
+ (if (types_match (@1, type) && int_fits_type_p (@2, type)
+ && TYPE_SIGN (TREE_TYPE (@0)) == TYPE_SIGN (type)
+ && TYPE_PRECISION (TREE_TYPE (@0)) > TYPE_PRECISION (type))
+ (minmax @1 (convert @2)))))
+
(for minmax (FMIN FMAX)
/* If either argument is NaN, return the other one. Avoid the
transformation if we get (and honor) a signalling NaN. */
diff --git a/gcc/testsuite/gcc.dg/fold-convmaxconv-1.c b/gcc/testsuite/gcc.dg/fold-convmaxconv-1.c
new file mode 100644
index 0000000..3ffff8b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fold-convmaxconv-1.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-optimized" } */
+
+int foo (short a[], int x)
+{
+ unsigned int i;
+ for (i = 0; i < 1000; i++)
+ {
+ x = a[i];
+ a[i] = (x <= 0 ? 0 : x);
+ }
+ return x;
+}
+
+/* { dg-final { scan-tree-dump-not " = MAX_EXPR <x_\[0-9\]*" "optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/fold-convminconv-1.c b/gcc/testsuite/gcc.dg/fold-convminconv-1.c
new file mode 100644
index 0000000..f4a048e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fold-convminconv-1.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-optimized" } */
+
+int foo (unsigned short a[], unsigned int x)
+{
+ unsigned int i;
+ for (i = 0; i < 1000; i++)
+ {
+ x = a[i];
+ a[i] = (x >= 255 ? 255 : x);
+ }
+ return x;
+}
+
+/* { dg-final { scan-tree-dump-not " = MIN_EXPR <x_\[0-9\]*" "optimized" } } */
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH GCC][1/4]Simplify (convert1 (minmax ((convert2 (x) c)))) into minmax (x c)
2016-10-26 14:31 ` Bin.Cheng
@ 2016-10-26 15:05 ` Marc Glisse
2016-10-26 15:22 ` Bin.Cheng
0 siblings, 1 reply; 9+ messages in thread
From: Marc Glisse @ 2016-10-26 15:05 UTC (permalink / raw)
To: Bin.Cheng; +Cc: gcc-patches List, Richard Biener
On Wed, 26 Oct 2016, Bin.Cheng wrote:
> On Wed, Oct 26, 2016 at 3:10 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
>> On Wed, Oct 26, 2016 at 3:04 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
>>> On Wed, 26 Oct 2016, Bin.Cheng wrote:
>>>
>>>> Thanks for reviewing, updated patch attached. Is it OK?
>>>
>>>
>>> +/* (convert (minmax ((convert (x) c)))) -> minmax (x c) if x is promoted
>>> + and the outer convert demotes the expression back to x's type. */
>>> +(for minmax (min max)
>>> + (simplify
>>> + (convert (minmax@0 (convert @1) INTEGER_CST@2))
>>> + (if (types_match (@1, type) && int_fits_type_p (@2, type)
>>> + && TYPE_PRECISION (TREE_TYPE (@0)) > TYPE_PRECISION (TREE_TYPE
>>> (@1)))
>>> + (minmax @1 (convert @2)))))
>>>
>>> Don't you have a problem if @1 is signed but not @0?
>>> (int)max((unsigned long)(-2),3ul)
>>> seems to satisfy your conditions, but is not the same as
>>> max(-2,3)
>> Ah, yes. I falsely removed sign check from the original patch. Will
>> update that.
>>
> Here it is. Sorry for the mistake.
I expect the issues are only with signed @1 and unsigned @0, the reverse
should be safe. But conservatively requiring the same TYPE_SIGN works if
you think the that covers enough cases.
(not a reviewer)
--
Marc Glisse
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH GCC][1/4]Simplify (convert1 (minmax ((convert2 (x) c)))) into minmax (x c)
2016-10-26 15:05 ` Marc Glisse
@ 2016-10-26 15:22 ` Bin.Cheng
2016-10-27 7:39 ` Richard Biener
0 siblings, 1 reply; 9+ messages in thread
From: Bin.Cheng @ 2016-10-26 15:22 UTC (permalink / raw)
To: gcc-patches List; +Cc: Richard Biener
On Wed, Oct 26, 2016 at 4:05 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Wed, 26 Oct 2016, Bin.Cheng wrote:
>
>> On Wed, Oct 26, 2016 at 3:10 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
>>>
>>> On Wed, Oct 26, 2016 at 3:04 PM, Marc Glisse <marc.glisse@inria.fr>
>>> wrote:
>>>>
>>>> On Wed, 26 Oct 2016, Bin.Cheng wrote:
>>>>
>>>>> Thanks for reviewing, updated patch attached. Is it OK?
>>>>
>>>>
>>>>
>>>> +/* (convert (minmax ((convert (x) c)))) -> minmax (x c) if x is
>>>> promoted
>>>> + and the outer convert demotes the expression back to x's type. */
>>>> +(for minmax (min max)
>>>> + (simplify
>>>> + (convert (minmax@0 (convert @1) INTEGER_CST@2))
>>>> + (if (types_match (@1, type) && int_fits_type_p (@2, type)
>>>> + && TYPE_PRECISION (TREE_TYPE (@0)) > TYPE_PRECISION (TREE_TYPE
>>>> (@1)))
>>>> + (minmax @1 (convert @2)))))
>>>>
>>>> Don't you have a problem if @1 is signed but not @0?
>>>> (int)max((unsigned long)(-2),3ul)
>>>> seems to satisfy your conditions, but is not the same as
>>>> max(-2,3)
>>>
>>> Ah, yes. I falsely removed sign check from the original patch. Will
>>> update that.
>>>
>> Here it is. Sorry for the mistake.
>
>
> I expect the issues are only with signed @1 and unsigned @0, the reverse
> should be safe. But conservatively requiring the same TYPE_SIGN works if you
> think the that covers enough cases.
It covers the motivation test case, but relaxed condition is of course
more useful. I will address this in followup patch extending this
pattern for non-const @2. Does this make sense?
Thanks,
bin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH GCC][1/4]Simplify (convert1 (minmax ((convert2 (x) c)))) into minmax (x c)
2016-10-26 15:22 ` Bin.Cheng
@ 2016-10-27 7:39 ` Richard Biener
0 siblings, 0 replies; 9+ messages in thread
From: Richard Biener @ 2016-10-27 7:39 UTC (permalink / raw)
To: Bin.Cheng; +Cc: gcc-patches List
On Wed, Oct 26, 2016 at 5:22 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
> On Wed, Oct 26, 2016 at 4:05 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
>> On Wed, 26 Oct 2016, Bin.Cheng wrote:
>>
>>> On Wed, Oct 26, 2016 at 3:10 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
>>>>
>>>> On Wed, Oct 26, 2016 at 3:04 PM, Marc Glisse <marc.glisse@inria.fr>
>>>> wrote:
>>>>>
>>>>> On Wed, 26 Oct 2016, Bin.Cheng wrote:
>>>>>
>>>>>> Thanks for reviewing, updated patch attached. Is it OK?
>>>>>
>>>>>
>>>>>
>>>>> +/* (convert (minmax ((convert (x) c)))) -> minmax (x c) if x is
>>>>> promoted
>>>>> + and the outer convert demotes the expression back to x's type. */
>>>>> +(for minmax (min max)
>>>>> + (simplify
>>>>> + (convert (minmax@0 (convert @1) INTEGER_CST@2))
>>>>> + (if (types_match (@1, type) && int_fits_type_p (@2, type)
>>>>> + && TYPE_PRECISION (TREE_TYPE (@0)) > TYPE_PRECISION (TREE_TYPE
>>>>> (@1)))
>>>>> + (minmax @1 (convert @2)))))
>>>>>
>>>>> Don't you have a problem if @1 is signed but not @0?
>>>>> (int)max((unsigned long)(-2),3ul)
>>>>> seems to satisfy your conditions, but is not the same as
>>>>> max(-2,3)
>>>>
>>>> Ah, yes. I falsely removed sign check from the original patch. Will
>>>> update that.
>>>>
>>> Here it is. Sorry for the mistake.
>>
>>
>> I expect the issues are only with signed @1 and unsigned @0, the reverse
>> should be safe. But conservatively requiring the same TYPE_SIGN works if you
>> think the that covers enough cases.
> It covers the motivation test case, but relaxed condition is of course
> more useful. I will address this in followup patch extending this
> pattern for non-const @2. Does this make sense?
Yes. The patch is ok.
Thanks,
Richard.
> Thanks,
> bin
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-10-27 7:39 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-25 11:21 [PATCH GCC][1/4]Simplify (convert1 (minmax ((convert2 (x) c)))) into minmax (x c) Bin Cheng
2016-10-25 11:48 ` Richard Biener
2016-10-26 13:51 ` Bin.Cheng
2016-10-26 14:04 ` Marc Glisse
2016-10-26 14:10 ` Bin.Cheng
2016-10-26 14:31 ` Bin.Cheng
2016-10-26 15:05 ` Marc Glisse
2016-10-26 15:22 ` Bin.Cheng
2016-10-27 7:39 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).