| Message ID | CAHFci289g7ttsCWdYbqrLH46tdLT5g15VnpEw-bcMeYr-La-kg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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)
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
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
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" } } */