Re: Fix up -fexcess-precision handling in LTO (was Re: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch))
On Tue, 30 Jul 2019, Jakub Jelinek wrote: > Furthermore, some comments claimed that the proper EXCESS_PRECISION_STANDARD > handling requires FE support, but that also doesn't seem to be the case > these days, some FEs even just use EXCESS_PRECISION_STANDARD by default > (go, D). > > So, the following patch gets rid of flag_excess_precision and renames > flag_excess_precision_cmdline to flag_excess_precision, plus adds > Optimization flag to that command line option, so that we remember it during > compilation and e.g. during LTO can then have some functions with standard > excess precision and others with fast excess precision. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? OK minus the removal of the comment in lhd_post_options. Proper handling requires front-end support (to generate GIMPLE with the operations in the intended type). Back-end support (to avoid having insn patterns claiming to operate on the types the processor in fact does not have direct support for arithmetic on), although not strictly required, is a very good idea, to make it more obvious if something is wrongly generating arithmetic on inappropriate types. And then you need the middle-end support (to avoid transformations introducing operations in the types that aren't meant to have direct operations, even if in fact the semantics are equivalent) as well. -- Joseph S. Myers jos...@codesourcery.com
Patch ping (Re: Fix up -fexcess-precision handling in LTO (was Re: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch)))
Hi! I'd like to ping the https://gcc.gnu.org/ml/gcc-patches/2019-07/msg01750.html patch. Thanks. On Tue, Jul 30, 2019 at 09:09:11AM +0200, Jakub Jelinek wrote: > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2019-07-30 Jakub Jelinek > > PR middle-end/91283 > * common.opt (fexcess-precision=): Add Optimization flag. Use > flag_excess_precision variable instead of > flag_excess_precision_cmdline. > * flags.h (class target_flag_state): Remove x_flag_excess_precision > member. > (flag_excess_precision): Don't define. > * langhooks.c (lhd_post_options): Set flag_excess_precision instead of > flag_excess_precision_cmdline. Remove comment. > * opts.c (set_fast_math_flags): Use frontend_set_flag_excess_precision > and x_flag_excess_precision instead of > frontend_set_flag_excess_precision_cmdline and > x_flag_excess_precision_cmdline. > (fast_math_flags_set_p): Use x_flag_excess_precision instead of > x_flag_excess_precision_cmdline. > * toplev.c (init_excess_precision): Remove. > (lang_dependent_init_target): Don't call it. > ada/ > * gcc-interface/misc.c (gnat_post_options): Set flag_excess_precision > instead of flag_excess_precision_cmdline. > brig/ > * brig-lang.c (brig_langhook_post_options): Set flag_excess_precision > instead of flag_excess_precision_cmdline. > c-family/ > * c-common.c (c_ts18661_flt_eval_method): Use flag_excess_precision > instead of flag_excess_precision_cmdline. > * c-cppbuiltin.c (c_cpp_flt_eval_method_iec_559): Likewise. > * c-opts.c (c_common_post_options): Likewise. > d/ > * d-lang.cc (d_post_options): Set flag_excess_precision instead of > flag_excess_precision_cmdline. > fortran/ > * options.c (gfc_post_options): Set flag_excess_precision instead of > flag_excess_precision_cmdline. Remove comment. > go/ > * go-lang.c (go_langhook_post_options): Set flag_excess_precision > instead of flag_excess_precision_cmdline. > lto/ > * lto-lang.c (lto_post_options): Set flag_excess_precision instead of > flag_excess_precision_cmdline. Remove comment. Jakub
Re: Fix up -fexcess-precision handling in LTO (was Re: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch))
On Tue, Jul 30, 2019 at 09:35:04AM +0200, Richard Biener wrote: > Looks OK to me but I'd like Joseph to chime in here. I think for Ok, will wait for Joseph. > FE support it means adding actual _widening_ casts to the effective > compute precision type (thus long double). But once that's done > the rest should be indeed FE independent (but I doubt D and go get > it correct). The FE must add casts to whatever excess_precision_type returns (if non-NULL). Seems all of C, D and Go do something: c/c-typeck.c: && (eptype = excess_precision_type (type2)) != NULL_TREE) c/c-typeck.c: && (eptype = excess_precision_type (type1)) != NULL_TREE) c/c-typeck.c: && (eptype = excess_precision_type (type0)) != NULL_TREE) c/c-typeck.c: && (eptype = excess_precision_type (type1)) != NULL_TREE) c-family/c-lex.c: const_type = excess_precision_type (type); d/expr.cc: tree eptype = excess_precision_type (type); d/expr.cc:tree eptype = excess_precision_type (type); go/go-gcc.cc:tree computed_type = excess_precision_type(type_tree); go/go-gcc.cc: tree computed_type = excess_precision_type(type_tree); go/go-gcc.cc: excess_type = excess_precision_type(TREE_TYPE(args[0])); match.pd: && !excess_precision_type (newtype))) and in the middle-end (since Tamar's changes) we only care if it returns NULL or not. Jakub
Re: Fix up -fexcess-precision handling in LTO (was Re: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch))
On Tue, 30 Jul 2019, Jakub Jelinek wrote: > On Tue, Jul 02, 2019 at 04:43:54PM +, Tamar Christina wrote: > > Here's an updated patch with the changes processed from the previous review. > > > > I've bootstrapped and regtested on aarch64-none-linux-gnu and > > x86_64-pc-linux-gnu and no issues. > > These changes also broke gcc.dg/torture/c99-contract-1.c with -flto > on i686-linux. > > The problem is that after moving the folding from convert.c to match.pd, > it is now performed not only during FE folding, but also much later on, > including post-IPA optimizations in lto1. The C FE arranges > flag_excess_precision_cmdline and flag_excess_precision to be > EXCESS_PRECISION_STANDARD and thus on i686-linux floating point arithmetics > is performed in long double, but the lto1 FE has both set to > EXCESS_PRECISION_FAST and undoes that widening. > > There seems to be quite complicated distinction between > flag_excess_precision_cmdline and flag_excess_precision, but it seems > that these days it is unnecessary, flag_excess_precision is only ever set > from flag_excess_precision_cmdline, perhaps in the past targets used to > modify flag_excess_precision, but they don't do that anymore. > > Furthermore, some comments claimed that the proper EXCESS_PRECISION_STANDARD > handling requires FE support, but that also doesn't seem to be the case > these days, some FEs even just use EXCESS_PRECISION_STANDARD by default > (go, D). > > So, the following patch gets rid of flag_excess_precision and renames > flag_excess_precision_cmdline to flag_excess_precision, plus adds > Optimization flag to that command line option, so that we remember it during > compilation and e.g. during LTO can then have some functions with standard > excess precision and others with fast excess precision. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Looks OK to me but I'd like Joseph to chime in here. I think for FE support it means adding actual _widening_ casts to the effective compute precision type (thus long double). But once that's done the rest should be indeed FE independent (but I doubt D and go get it correct). Thanks, Richard. > 2019-07-30 Jakub Jelinek > > PR middle-end/91283 > * common.opt (fexcess-precision=): Add Optimization flag. Use > flag_excess_precision variable instead of > flag_excess_precision_cmdline. > * flags.h (class target_flag_state): Remove x_flag_excess_precision > member. > (flag_excess_precision): Don't define. > * langhooks.c (lhd_post_options): Set flag_excess_precision instead of > flag_excess_precision_cmdline. Remove comment. > * opts.c (set_fast_math_flags): Use frontend_set_flag_excess_precision > and x_flag_excess_precision instead of > frontend_set_flag_excess_precision_cmdline and > x_flag_excess_precision_cmdline. > (fast_math_flags_set_p): Use x_flag_excess_precision instead of > x_flag_excess_precision_cmdline. > * toplev.c (init_excess_precision): Remove. > (lang_dependent_init_target): Don't call it. > ada/ > * gcc-interface/misc.c (gnat_post_options): Set flag_excess_precision > instead of flag_excess_precision_cmdline. > brig/ > * brig-lang.c (brig_langhook_post_options): Set flag_excess_precision > instead of flag_excess_precision_cmdline. > c-family/ > * c-common.c (c_ts18661_flt_eval_method): Use flag_excess_precision > instead of flag_excess_precision_cmdline. > * c-cppbuiltin.c (c_cpp_flt_eval_method_iec_559): Likewise. > * c-opts.c (c_common_post_options): Likewise. > d/ > * d-lang.cc (d_post_options): Set flag_excess_precision instead of > flag_excess_precision_cmdline. > fortran/ > * options.c (gfc_post_options): Set flag_excess_precision instead of > flag_excess_precision_cmdline. Remove comment. > go/ > * go-lang.c (go_langhook_post_options): Set flag_excess_precision > instead of flag_excess_precision_cmdline. > lto/ > * lto-lang.c (lto_post_options): Set flag_excess_precision instead of > flag_excess_precision_cmdline. Remove comment. > > --- gcc/common.opt.jj 2019-07-29 12:56:38.968248060 +0200 > +++ gcc/common.opt2019-07-29 13:01:24.067067583 +0200 > @@ -1399,7 +1399,7 @@ Common Report Var(flag_expensive_optimiz > Perform a number of minor, expensive optimizations. > > fexcess-precision= > -Common Joined RejectNegative Enum(excess_precision) > Var(flag_excess_precision_cmdline) Init(EXCESS_PRECISION_DEFAULT) > SetByCombined > +Common Joined RejectNegative Enum(excess_precision) > Var(flag_excess_precision) Init(EXCESS_PRECISION_DEFAULT) Optimization > SetByCombined > -fexcess-precision=[fast|standard] Specify handling of excess > floating-point precision. > > Enum > --- gcc/flags.h.jj2019-07-10 15:52:20.362155642 +0200 > +++ gcc/flags.h 2019-07-29 13:02:05.488460207 +0200 > @@ -51,9 +51,6 @@ public: >
Re: [PATCH] Fix up gcc.dg/type-convert-var.c testcase (was Re: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch))
On Tue, 30 Jul 2019, Jakub Jelinek wrote:
> On Tue, Jul 30, 2019 at 09:05:35AM +0200, Rainer Orth wrote:
> > > This new testcase FAILs e.g. on i686-linux. The problem is that
> >
> > this is PR middle-end/91282.
>
> Indeed.
>
> > > with no dg-options, the testcase options default to -ansi, which
> > > implies -fexcess-precision=standard. On i686-linux, that is conversion to
> > > long double which must (and does) survive until expansion.
> > >
> > > Fixed by using -fexcess-precision=fast, tested on x86_64-linux and
> > > i686-linux, ok for trunk?
> > >
> > > 2019-07-30 Jakub Jelinek
> > >
> > > * gcc.dg/type-convert-var.c: Add -0fexcess-precision=fast to
> > ^ typo
>
> Oops, fixed:
OK.
> 2019-07-30 Jakub Jelinek
>
> PR middle-end/91282
> * gcc.dg/type-convert-var.c: Add -fexcess-precision=fast to
> dg-additional-options.
>
> --- gcc/testsuite/gcc.dg/type-convert-var.c.jj2019-07-28
> 17:29:27.156351325 +0200
> +++ gcc/testsuite/gcc.dg/type-convert-var.c 2019-07-30 08:51:33.349558035
> +0200
> @@ -1,5 +1,5 @@
> /* { dg-do compile } */
> -/* { dg-additional-options "-O1 -fdump-tree-optimized" } */
> +/* { dg-additional-options "-fexcess-precision=fast -O1
> -fdump-tree-optimized" } */
> void foo (float a, float b, float *c)
> {
>double e = (double)a * (double)b;
>
>
> Jakub
>
--
Richard Biener
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany;
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg)
Re: [PATCH] Fix up gcc.dg/type-convert-var.c testcase (was Re: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch))
On Tue, Jul 30, 2019 at 09:05:35AM +0200, Rainer Orth wrote:
> > This new testcase FAILs e.g. on i686-linux. The problem is that
>
> this is PR middle-end/91282.
Indeed.
> > with no dg-options, the testcase options default to -ansi, which
> > implies -fexcess-precision=standard. On i686-linux, that is conversion to
> > long double which must (and does) survive until expansion.
> >
> > Fixed by using -fexcess-precision=fast, tested on x86_64-linux and
> > i686-linux, ok for trunk?
> >
> > 2019-07-30 Jakub Jelinek
> >
> > * gcc.dg/type-convert-var.c: Add -0fexcess-precision=fast to
> ^ typo
Oops, fixed:
2019-07-30 Jakub Jelinek
PR middle-end/91282
* gcc.dg/type-convert-var.c: Add -fexcess-precision=fast to
dg-additional-options.
--- gcc/testsuite/gcc.dg/type-convert-var.c.jj 2019-07-28 17:29:27.156351325
+0200
+++ gcc/testsuite/gcc.dg/type-convert-var.c 2019-07-30 08:51:33.349558035
+0200
@@ -1,5 +1,5 @@
/* { dg-do compile } */
-/* { dg-additional-options "-O1 -fdump-tree-optimized" } */
+/* { dg-additional-options "-fexcess-precision=fast -O1 -fdump-tree-optimized"
} */
void foo (float a, float b, float *c)
{
double e = (double)a * (double)b;
Jakub
Fix up -fexcess-precision handling in LTO (was Re: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch))
On Tue, Jul 02, 2019 at 04:43:54PM +, Tamar Christina wrote: > Here's an updated patch with the changes processed from the previous review. > > I've bootstrapped and regtested on aarch64-none-linux-gnu and > x86_64-pc-linux-gnu and no issues. These changes also broke gcc.dg/torture/c99-contract-1.c with -flto on i686-linux. The problem is that after moving the folding from convert.c to match.pd, it is now performed not only during FE folding, but also much later on, including post-IPA optimizations in lto1. The C FE arranges flag_excess_precision_cmdline and flag_excess_precision to be EXCESS_PRECISION_STANDARD and thus on i686-linux floating point arithmetics is performed in long double, but the lto1 FE has both set to EXCESS_PRECISION_FAST and undoes that widening. There seems to be quite complicated distinction between flag_excess_precision_cmdline and flag_excess_precision, but it seems that these days it is unnecessary, flag_excess_precision is only ever set from flag_excess_precision_cmdline, perhaps in the past targets used to modify flag_excess_precision, but they don't do that anymore. Furthermore, some comments claimed that the proper EXCESS_PRECISION_STANDARD handling requires FE support, but that also doesn't seem to be the case these days, some FEs even just use EXCESS_PRECISION_STANDARD by default (go, D). So, the following patch gets rid of flag_excess_precision and renames flag_excess_precision_cmdline to flag_excess_precision, plus adds Optimization flag to that command line option, so that we remember it during compilation and e.g. during LTO can then have some functions with standard excess precision and others with fast excess precision. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2019-07-30 Jakub Jelinek PR middle-end/91283 * common.opt (fexcess-precision=): Add Optimization flag. Use flag_excess_precision variable instead of flag_excess_precision_cmdline. * flags.h (class target_flag_state): Remove x_flag_excess_precision member. (flag_excess_precision): Don't define. * langhooks.c (lhd_post_options): Set flag_excess_precision instead of flag_excess_precision_cmdline. Remove comment. * opts.c (set_fast_math_flags): Use frontend_set_flag_excess_precision and x_flag_excess_precision instead of frontend_set_flag_excess_precision_cmdline and x_flag_excess_precision_cmdline. (fast_math_flags_set_p): Use x_flag_excess_precision instead of x_flag_excess_precision_cmdline. * toplev.c (init_excess_precision): Remove. (lang_dependent_init_target): Don't call it. ada/ * gcc-interface/misc.c (gnat_post_options): Set flag_excess_precision instead of flag_excess_precision_cmdline. brig/ * brig-lang.c (brig_langhook_post_options): Set flag_excess_precision instead of flag_excess_precision_cmdline. c-family/ * c-common.c (c_ts18661_flt_eval_method): Use flag_excess_precision instead of flag_excess_precision_cmdline. * c-cppbuiltin.c (c_cpp_flt_eval_method_iec_559): Likewise. * c-opts.c (c_common_post_options): Likewise. d/ * d-lang.cc (d_post_options): Set flag_excess_precision instead of flag_excess_precision_cmdline. fortran/ * options.c (gfc_post_options): Set flag_excess_precision instead of flag_excess_precision_cmdline. Remove comment. go/ * go-lang.c (go_langhook_post_options): Set flag_excess_precision instead of flag_excess_precision_cmdline. lto/ * lto-lang.c (lto_post_options): Set flag_excess_precision instead of flag_excess_precision_cmdline. Remove comment. --- gcc/common.opt.jj 2019-07-29 12:56:38.968248060 +0200 +++ gcc/common.opt 2019-07-29 13:01:24.067067583 +0200 @@ -1399,7 +1399,7 @@ Common Report Var(flag_expensive_optimiz Perform a number of minor, expensive optimizations. fexcess-precision= -Common Joined RejectNegative Enum(excess_precision) Var(flag_excess_precision_cmdline) Init(EXCESS_PRECISION_DEFAULT) SetByCombined +Common Joined RejectNegative Enum(excess_precision) Var(flag_excess_precision) Init(EXCESS_PRECISION_DEFAULT) Optimization SetByCombined -fexcess-precision=[fast|standard] Specify handling of excess floating-point precision. Enum --- gcc/flags.h.jj 2019-07-10 15:52:20.362155642 +0200 +++ gcc/flags.h 2019-07-29 13:02:05.488460207 +0200 @@ -51,9 +51,6 @@ public: align_flags x_align_jumps; align_flags x_align_labels; align_flags x_align_functions; - - /* The excess precision currently in effect. */ - enum excess_precision x_flag_excess_precision; }; extern class target_flag_state default_target_flag_state; @@ -68,12 +65,6 @@ extern class target_flag_state *this_tar #define align_labels(this_target_flag_state->x_align_labels) #define align_functions (this_target_flag_state->x_align_functions) -/*
Re: [PATCH] Fix up gcc.dg/type-convert-var.c testcase (was Re: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch))
Hi Jakub,
> On Tue, Jul 02, 2019 at 04:43:54PM +, Tamar Christina wrote:
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/type-convert-var.c
>> @@ -0,0 +1,9 @@
>> +/* { dg-do compile } */
>> +/* { dg-additional-options "-O1 -fdump-tree-optimized" } */
>> +void foo (float a, float b, float *c)
>> +{
>> + double e = (double)a * (double)b;
>> + *c = (float)e;
>> +}
>> +
>> +/* { dg-final { scan-tree-dump-not {double} "optimized" } } */
>>
>
> This new testcase FAILs e.g. on i686-linux. The problem is that
this is PR middle-end/91282.
> with no dg-options, the testcase options default to -ansi, which
> implies -fexcess-precision=standard. On i686-linux, that is conversion to
> long double which must (and does) survive until expansion.
>
> Fixed by using -fexcess-precision=fast, tested on x86_64-linux and
> i686-linux, ok for trunk?
>
> 2019-07-30 Jakub Jelinek
>
> * gcc.dg/type-convert-var.c: Add -0fexcess-precision=fast to
^ typo
Rainer
--
-
Rainer Orth, Center for Biotechnology, Bielefeld University
[PATCH] Fix up gcc.dg/type-convert-var.c testcase (was Re: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch))
On Tue, Jul 02, 2019 at 04:43:54PM +, Tamar Christina wrote:
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/type-convert-var.c
> @@ -0,0 +1,9 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-O1 -fdump-tree-optimized" } */
> +void foo (float a, float b, float *c)
> +{
> + double e = (double)a * (double)b;
> + *c = (float)e;
> +}
> +
> +/* { dg-final { scan-tree-dump-not {double} "optimized" } } */
>
This new testcase FAILs e.g. on i686-linux. The problem is that
with no dg-options, the testcase options default to -ansi, which
implies -fexcess-precision=standard. On i686-linux, that is conversion to
long double which must (and does) survive until expansion.
Fixed by using -fexcess-precision=fast, tested on x86_64-linux and
i686-linux, ok for trunk?
2019-07-30 Jakub Jelinek
* gcc.dg/type-convert-var.c: Add -0fexcess-precision=fast to
dg-additional-options.
--- gcc/testsuite/gcc.dg/type-convert-var.c.jj 2019-07-28 17:29:27.156351325
+0200
+++ gcc/testsuite/gcc.dg/type-convert-var.c 2019-07-30 08:51:33.349558035
+0200
@@ -1,5 +1,5 @@
/* { dg-do compile } */
-/* { dg-additional-options "-O1 -fdump-tree-optimized" } */
+/* { dg-additional-options "-fexcess-precision=fast -O1 -fdump-tree-optimized"
} */
void foo (float a, float b, float *c)
{
double e = (double)a * (double)b;
Jakub
Re: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch)
On Tue, 2 Jul 2019, Tamar Christina wrote:
>
> Hi All,
>
> Here's an updated patch with the changes processed from the previous review.
>
> I've bootstrapped and regtested on aarch64-none-linux-gnu and
> x86_64-pc-linux-gnu and no issues.
>
> Ok for trunk?
+ (if (TYPE_OVERFLOW_WRAPS (TREE_TYPE (@1)))
+ (op @1 (convert @2))
+ (with { tree utype = unsigned_type_for (TREE_TYPE (@1)); }
+ (convert (op (convert:utype @1)
indenting is off, one space more for (convert (it's inside the with)
+ (convert:utype @2)
+ (with { tree arg0 = strip_float_extensions (@1);
indenting is off here, one space less for the (with please.
you'll run into strip_float_extensions for integer types as well so
please move the FLOAT_TYPE_P (type) check before the with like
(if (FLOAT_TYPE_P (type)
&& DECIMAL_FLOAT_TYPE_P (TREE_TPE (@0)) == DECIMAL_FLOAT_TYPE_P
(type))
(with { tree arg0 = strip_float_extensions (@1);
+ (if ((newtype == dfloat32_type_node
+ || newtype == dfloat64_type_node
+ || newtype == dfloat128_type_node)
+ && newtype == type)
+ (convert:newtype (op (convert:newtype @1) (convert:newtype
@2)))
I think you want to elide the outermost convert:newtype here and use
(op (convert:newtype @1) (convert:newtype @2))
newtype == type check you also want to write types_match_p (newtype, type)
+ (if (TYPE_PRECISION (newtype) < TYPE_PRECISION (itype)
+ && (flag_unsafe_math_optimizations
+ || (TYPE_PRECISION (newtype) == TYPE_PRECISION
(type)
+ && real_can_shorten_arithmetic (TYPE_MODE
(itype),
+ TYPE_MODE
(type))
+ && !excess_precision_type (newtype
+(convert:newtype (op (convert:newtype @1)
+ (convert:newtype @2)))
here the outermost convert looks bogus - you need to build an
expression of type 'type' thus
(convert:type (op (convert:newtype @1) (convert:newtype @2)))
I think you also want to avoid endless recursion by adding a
&& !types_match_p (itype, newtype)
since in that case you've re-created the original expression.
OK with those changes.
Thanks,
Richard.
> Thanks,
> Tamar
>
> The 07/02/2019 11:20, Richard Biener wrote:
> > On Tue, 2 Jul 2019, Tamar Christina wrote:
> >
> > > Hi Richi,
> > >
> > > The 06/25/2019 10:02, Richard Biener wrote:
> > > >
> > > > This looks like a literal 1:1 translation plus merging with the
> > > > existing pattern around integers. You changed
> > > > (op:s@0 (convert@3 @1) (convert?@4 @2)) to
> > > > (op:s@0 (convert1?@3 @1) (convert2?@4 @2)) where this now also
> > > > matches if there are no inner conversions at all - was that a
> > > > desired change or did you merely want to catch when the first
> > > > operand is not a conversion but the second is, possibly only
> > > > for the RDIV_EXPR case?
> > > >
> > >
> > > Yes, the ? ? is for the RDIV case, I really only want (c a) `op` (c b),
> > > a `op` (c b) and (c a) `op` b. But didn't find a way to do this.
> >
> > One way would be to do
> >
> > (simplify
> > (convert (op:sc@0 (convert @1) (convert? @2)))
> >
> > but that doesn't work for RDIV. Using :C is tempting but you
> > do not get to know the original operand order which you of
> > course need. So I guess the way you do it is fine - you
> > could guard all of the code with a few types_match () checks
> > but I'm not sure it is worth the trouble.
> >
> > Richard.
> >
> > > The only thing I can think of that gets this is without overmatching is
> > > to either duplicate the code or move this back to a C helper function and
> > > call that from match.pd. But I was hoping to have it all in match.pd
> > > instead of hiding it away in a C call.
> > >
> > > Do you have a better way of doing it or a preference to an approach?
> > >
> > > > +(for op (plus minus mult rdiv)
> > > > + (simplify
> > > > + (convert (op:s@0 (convert1?@3 @1) (convert2?@4 @2)))
> > > > + (with { tree arg0 = strip_float_extensions (@1);
> > > > + tree arg1 = strip_float_extensions (@2);
> > > > + tree itype = TREE_TYPE (@0);
> > > >
> > > > you now unconditionally call strip_float_extensions on each operand
> > > > even for the integer case, please sink stuff only used in one
> > > > case arm. I guess keeping the integer case first via
> > > >
> > >
> > > Done, Initially didn't think it would be an issue since I don't use the
> > > value it
> > > creates in the integer case. But I re-ordered it.
> > >
> > > > should work (with the 'with' being in the ifs else position).
> > > >
> > > > + (if (code == REAL_TYPE)
> > > > + /* Ignore the conversion if we don't need to store intermediate
> > > > + results and neither type is a decimal float. */
> > > > +
Re: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch)
Hi All,
Here's an updated patch with the changes processed from the previous review.
I've bootstrapped and regtested on aarch64-none-linux-gnu and
x86_64-pc-linux-gnu and no issues.
Ok for trunk?
Thanks,
Tamar
The 07/02/2019 11:20, Richard Biener wrote:
> On Tue, 2 Jul 2019, Tamar Christina wrote:
>
> > Hi Richi,
> >
> > The 06/25/2019 10:02, Richard Biener wrote:
> > >
> > > This looks like a literal 1:1 translation plus merging with the
> > > existing pattern around integers. You changed
> > > (op:s@0 (convert@3 @1) (convert?@4 @2)) to
> > > (op:s@0 (convert1?@3 @1) (convert2?@4 @2)) where this now also
> > > matches if there are no inner conversions at all - was that a
> > > desired change or did you merely want to catch when the first
> > > operand is not a conversion but the second is, possibly only
> > > for the RDIV_EXPR case?
> > >
> >
> > Yes, the ? ? is for the RDIV case, I really only want (c a) `op` (c b),
> > a `op` (c b) and (c a) `op` b. But didn't find a way to do this.
>
> One way would be to do
>
> (simplify
> (convert (op:sc@0 (convert @1) (convert? @2)))
>
> but that doesn't work for RDIV. Using :C is tempting but you
> do not get to know the original operand order which you of
> course need. So I guess the way you do it is fine - you
> could guard all of the code with a few types_match () checks
> but I'm not sure it is worth the trouble.
>
> Richard.
>
> > The only thing I can think of that gets this is without overmatching is
> > to either duplicate the code or move this back to a C helper function and
> > call that from match.pd. But I was hoping to have it all in match.pd
> > instead of hiding it away in a C call.
> >
> > Do you have a better way of doing it or a preference to an approach?
> >
> > > +(for op (plus minus mult rdiv)
> > > + (simplify
> > > + (convert (op:s@0 (convert1?@3 @1) (convert2?@4 @2)))
> > > + (with { tree arg0 = strip_float_extensions (@1);
> > > + tree arg1 = strip_float_extensions (@2);
> > > + tree itype = TREE_TYPE (@0);
> > >
> > > you now unconditionally call strip_float_extensions on each operand
> > > even for the integer case, please sink stuff only used in one
> > > case arm. I guess keeping the integer case first via
> > >
> >
> > Done, Initially didn't think it would be an issue since I don't use the
> > value it
> > creates in the integer case. But I re-ordered it.
> >
> > > should work (with the 'with' being in the ifs else position).
> > >
> > > + (if (code == REAL_TYPE)
> > > + /* Ignore the conversion if we don't need to store intermediate
> > > + results and neither type is a decimal float. */
> > > + (if (!(flag_float_store
> > > + || DECIMAL_FLOAT_TYPE_P (type)
> > > + || DECIMAL_FLOAT_TYPE_P (itype))
> > > + && types_match (ty1, ty2))
> > > + (convert (op (convert:ty1 @1) (convert:ty2 @2)
> > >
> > > this looks prone to the same recursion issue you described above.
> >
> > It's to break the recursion when you don't match anything. Indeed don't
> > need it if
> > I change the match condition above.
> > Thanks,
> > Tamar
> > >
> > > 'code' is used exactly once, using SCALAR_FLOAT_TYPE_P (itype)
> > > in the above test would be clearer. Also both ifs can be combined.
> > > The snipped above also doesn't appear in the convert.c code you
> > > remove and the original one is
> > >
> > > switch (TREE_CODE (TREE_TYPE (expr)))
> > > {
> > > case REAL_TYPE:
> > > /* Ignore the conversion if we don't need to store intermediate
> > > results and neither type is a decimal float. */
> > > return build1_loc (loc,
> > > (flag_float_store
> > > || DECIMAL_FLOAT_TYPE_P (type)
> > > || DECIMAL_FLOAT_TYPE_P (itype))
> > > ? CONVERT_EXPR : NOP_EXPR, type, expr);
> > >
> > > which as far as I can see doesn't do anything besides
> > > exchanging CONVERT_EXPR for NOP_EXPR which is a noop to the IL.
> > > So it appears this shouldn't be moved to match.pd at all?
> > > It's also not a 1:1 move since you are changing 'expr'.
> > >
> > > Thanks,
> > > Richard.
> > >
> > > > > Thanks,
> > > > > Tamar
> > > > >
> > > > > Concretely it makes both these cases behave the same
> > > > >
> > > > > float e = (float)a * (float)b;
> > > > > *c = (_Float16)e;
> > > > >
> > > > > and
> > > > >
> > > > > *c = (_Float16)((float)a * (float)b);
> > > > >
> > > > > Thanks,
> > > > > Tamar
> > > > >
> > > > > gcc/ChangeLog:
> > > > >
> > > > > 2019-06-25 Tamar Christina
> > > > >
> > > > > * convert.c (convert_to_real_1): Move part of conversion code...
> > > > > * match.pd: ...To here.
> > > > >
> > > > > gcc/testsuite/ChangeLog:
> > > > >
> > > > > 2019-06-25 Tamar Christina
> > > > >
> > > > > * gcc.dg/type-convert-var.c: New test.
> > > > >
> > > > > --
> > > >
Re: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch)
On Tue, 2 Jul 2019, Tamar Christina wrote:
> Hi Richi,
>
> The 06/25/2019 10:02, Richard Biener wrote:
> >
> > This looks like a literal 1:1 translation plus merging with the
> > existing pattern around integers. You changed
> > (op:s@0 (convert@3 @1) (convert?@4 @2)) to
> > (op:s@0 (convert1?@3 @1) (convert2?@4 @2)) where this now also
> > matches if there are no inner conversions at all - was that a
> > desired change or did you merely want to catch when the first
> > operand is not a conversion but the second is, possibly only
> > for the RDIV_EXPR case?
> >
>
> Yes, the ? ? is for the RDIV case, I really only want (c a) `op` (c b),
> a `op` (c b) and (c a) `op` b. But didn't find a way to do this.
One way would be to do
(simplify
(convert (op:sc@0 (convert @1) (convert? @2)))
but that doesn't work for RDIV. Using :C is tempting but you
do not get to know the original operand order which you of
course need. So I guess the way you do it is fine - you
could guard all of the code with a few types_match () checks
but I'm not sure it is worth the trouble.
Richard.
> The only thing I can think of that gets this is without overmatching is
> to either duplicate the code or move this back to a C helper function and
> call that from match.pd. But I was hoping to have it all in match.pd
> instead of hiding it away in a C call.
>
> Do you have a better way of doing it or a preference to an approach?
>
> > +(for op (plus minus mult rdiv)
> > + (simplify
> > + (convert (op:s@0 (convert1?@3 @1) (convert2?@4 @2)))
> > + (with { tree arg0 = strip_float_extensions (@1);
> > + tree arg1 = strip_float_extensions (@2);
> > + tree itype = TREE_TYPE (@0);
> >
> > you now unconditionally call strip_float_extensions on each operand
> > even for the integer case, please sink stuff only used in one
> > case arm. I guess keeping the integer case first via
> >
>
> Done, Initially didn't think it would be an issue since I don't use the value
> it
> creates in the integer case. But I re-ordered it.
>
> > should work (with the 'with' being in the ifs else position).
> >
> > + (if (code == REAL_TYPE)
> > + /* Ignore the conversion if we don't need to store intermediate
> > + results and neither type is a decimal float. */
> > + (if (!(flag_float_store
> > + || DECIMAL_FLOAT_TYPE_P (type)
> > + || DECIMAL_FLOAT_TYPE_P (itype))
> > + && types_match (ty1, ty2))
> > + (convert (op (convert:ty1 @1) (convert:ty2 @2)
> >
> > this looks prone to the same recursion issue you described above.
>
> It's to break the recursion when you don't match anything. Indeed don't need
> it if
> I change the match condition above.
> Thanks,
> Tamar
> >
> > 'code' is used exactly once, using SCALAR_FLOAT_TYPE_P (itype)
> > in the above test would be clearer. Also both ifs can be combined.
> > The snipped above also doesn't appear in the convert.c code you
> > remove and the original one is
> >
> > switch (TREE_CODE (TREE_TYPE (expr)))
> > {
> > case REAL_TYPE:
> > /* Ignore the conversion if we don't need to store intermediate
> > results and neither type is a decimal float. */
> > return build1_loc (loc,
> > (flag_float_store
> > || DECIMAL_FLOAT_TYPE_P (type)
> > || DECIMAL_FLOAT_TYPE_P (itype))
> > ? CONVERT_EXPR : NOP_EXPR, type, expr);
> >
> > which as far as I can see doesn't do anything besides
> > exchanging CONVERT_EXPR for NOP_EXPR which is a noop to the IL.
> > So it appears this shouldn't be moved to match.pd at all?
> > It's also not a 1:1 move since you are changing 'expr'.
> >
> > Thanks,
> > Richard.
> >
> > > > Thanks,
> > > > Tamar
> > > >
> > > > Concretely it makes both these cases behave the same
> > > >
> > > > float e = (float)a * (float)b;
> > > > *c = (_Float16)e;
> > > >
> > > > and
> > > >
> > > > *c = (_Float16)((float)a * (float)b);
> > > >
> > > > Thanks,
> > > > Tamar
> > > >
> > > > gcc/ChangeLog:
> > > >
> > > > 2019-06-25 Tamar Christina
> > > >
> > > > * convert.c (convert_to_real_1): Move part of conversion code...
> > > > * match.pd: ...To here.
> > > >
> > > > gcc/testsuite/ChangeLog:
> > > >
> > > > 2019-06-25 Tamar Christina
> > > >
> > > > * gcc.dg/type-convert-var.c: New test.
> > > >
> > > > --
> > >
> >
> > --
> > Richard Biener
> > SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany;
> > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg)
>
>
--
Richard Biener
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany;
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg)
Re: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch)
Hi Richi,
The 06/25/2019 10:02, Richard Biener wrote:
>
> This looks like a literal 1:1 translation plus merging with the
> existing pattern around integers. You changed
> (op:s@0 (convert@3 @1) (convert?@4 @2)) to
> (op:s@0 (convert1?@3 @1) (convert2?@4 @2)) where this now also
> matches if there are no inner conversions at all - was that a
> desired change or did you merely want to catch when the first
> operand is not a conversion but the second is, possibly only
> for the RDIV_EXPR case?
>
Yes, the ? ? is for the RDIV case, I really only want (c a) `op` (c b),
a `op` (c b) and (c a) `op` b. But didn't find a way to do this.
The only thing I can think of that gets this is without overmatching is
to either duplicate the code or move this back to a C helper function and
call that from match.pd. But I was hoping to have it all in match.pd
instead of hiding it away in a C call.
Do you have a better way of doing it or a preference to an approach?
> +(for op (plus minus mult rdiv)
> + (simplify
> + (convert (op:s@0 (convert1?@3 @1) (convert2?@4 @2)))
> + (with { tree arg0 = strip_float_extensions (@1);
> + tree arg1 = strip_float_extensions (@2);
> + tree itype = TREE_TYPE (@0);
>
> you now unconditionally call strip_float_extensions on each operand
> even for the integer case, please sink stuff only used in one
> case arm. I guess keeping the integer case first via
>
Done, Initially didn't think it would be an issue since I don't use the value it
creates in the integer case. But I re-ordered it.
> should work (with the 'with' being in the ifs else position).
>
> + (if (code == REAL_TYPE)
> + /* Ignore the conversion if we don't need to store intermediate
> + results and neither type is a decimal float. */
> + (if (!(flag_float_store
> + || DECIMAL_FLOAT_TYPE_P (type)
> + || DECIMAL_FLOAT_TYPE_P (itype))
> + && types_match (ty1, ty2))
> + (convert (op (convert:ty1 @1) (convert:ty2 @2)
>
> this looks prone to the same recursion issue you described above.
It's to break the recursion when you don't match anything. Indeed don't need it
if
I change the match condition above.
Thanks,
Tamar
>
> 'code' is used exactly once, using SCALAR_FLOAT_TYPE_P (itype)
> in the above test would be clearer. Also both ifs can be combined.
> The snipped above also doesn't appear in the convert.c code you
> remove and the original one is
>
> switch (TREE_CODE (TREE_TYPE (expr)))
> {
> case REAL_TYPE:
> /* Ignore the conversion if we don't need to store intermediate
> results and neither type is a decimal float. */
> return build1_loc (loc,
> (flag_float_store
> || DECIMAL_FLOAT_TYPE_P (type)
> || DECIMAL_FLOAT_TYPE_P (itype))
> ? CONVERT_EXPR : NOP_EXPR, type, expr);
>
> which as far as I can see doesn't do anything besides
> exchanging CONVERT_EXPR for NOP_EXPR which is a noop to the IL.
> So it appears this shouldn't be moved to match.pd at all?
> It's also not a 1:1 move since you are changing 'expr'.
>
> Thanks,
> Richard.
>
> > > Thanks,
> > > Tamar
> > >
> > > Concretely it makes both these cases behave the same
> > >
> > > float e = (float)a * (float)b;
> > > *c = (_Float16)e;
> > >
> > > and
> > >
> > > *c = (_Float16)((float)a * (float)b);
> > >
> > > Thanks,
> > > Tamar
> > >
> > > gcc/ChangeLog:
> > >
> > > 2019-06-25 Tamar Christina
> > >
> > > * convert.c (convert_to_real_1): Move part of conversion code...
> > > * match.pd: ...To here.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > > 2019-06-25 Tamar Christina
> > >
> > > * gcc.dg/type-convert-var.c: New test.
> > >
> > > --
> >
>
> --
> Richard Biener
> SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany;
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg)
--
RE: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch)
On Tue, 25 Jun 2019, Tamar Christina wrote:
> Adding some maintainers
>
> > -Original Message-
> > From: gcc-patches-ow...@gcc.gnu.org On
> > Behalf Of Tamar Christina
> > Sent: Tuesday, June 25, 2019 09:31
> > To: gcc-patches@gcc.gnu.org
> > Cc: nd
> > Subject: [GCC][middle-end] Add rules to strip away unneeded type casts
> > in expressions (2nd patch)
> >
> > Hi All,
> >
> > This is an updated version of my GCC-9 patch which moves part of the
> > type conversion code from convert.c to match.pd because match.pd is
> > able to apply this transformation in the presence of intermediate
> > temporary variables.
> >
> > The previous patch was only regtested on aarch64-none-linux-gnu and I
> > hadn't done a regression on x86_64-pc-linux-gnu only a bootstrap. The
> > previous patch was approved
> >
> > here https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00116.html
> > but before committing I ran a x86_64-pc-linux-gnu regtest to be sure
> > and this showed an issue with a DFP test. I Have fixed this by
> > removing the offending convert.
> > The convert was just saying "keep the type as is" but match.pd looped
> > here as it thinks the match did something and would try other
> > patterns, causing it to match itself again.
> >
> > Instead when there's nothing to update, I just don't do anything.
> >
> > The second change was to merge this with the existing pattern for
> > integer conversion in order to silence a warning from match.pd which
> > though that the two patterns overlaps because their match conditions
> > are similar (they have different conditions inside the ifs but
> > match.pd doesn't check those of course.).
> >
> > Regtested and bootstrapped on aarch64-none-linux-gnu and x86_64-pc-
> > linux-gnu and no issues.
> >
> > Ok for trunk?
This looks like a literal 1:1 translation plus merging with the
existing pattern around integers. You changed
(op:s@0 (convert@3 @1) (convert?@4 @2)) to
(op:s@0 (convert1?@3 @1) (convert2?@4 @2)) where this now also
matches if there are no inner conversions at all - was that a
desired change or did you merely want to catch when the first
operand is not a conversion but the second is, possibly only
for the RDIV_EXPR case?
+(for op (plus minus mult rdiv)
+ (simplify
+ (convert (op:s@0 (convert1?@3 @1) (convert2?@4 @2)))
+ (with { tree arg0 = strip_float_extensions (@1);
+ tree arg1 = strip_float_extensions (@2);
+ tree itype = TREE_TYPE (@0);
you now unconditionally call strip_float_extensions on each operand
even for the integer case, please sink stuff only used in one
case arm. I guess keeping the integer case first via
(if (INTEGRAL_TYPE_P (type)
...
(with { tree arg0 = strip_float_extensions (@1);
...
should work (with the 'with' being in the ifs else position).
+ (if (code == REAL_TYPE)
+ /* Ignore the conversion if we don't need to store intermediate
+ results and neither type is a decimal float. */
+ (if (!(flag_float_store
+ || DECIMAL_FLOAT_TYPE_P (type)
+ || DECIMAL_FLOAT_TYPE_P (itype))
+ && types_match (ty1, ty2))
+ (convert (op (convert:ty1 @1) (convert:ty2 @2)
this looks prone to the same recursion issue you described above.
'code' is used exactly once, using SCALAR_FLOAT_TYPE_P (itype)
in the above test would be clearer. Also both ifs can be combined.
The snipped above also doesn't appear in the convert.c code you
remove and the original one is
switch (TREE_CODE (TREE_TYPE (expr)))
{
case REAL_TYPE:
/* Ignore the conversion if we don't need to store intermediate
results and neither type is a decimal float. */
return build1_loc (loc,
(flag_float_store
|| DECIMAL_FLOAT_TYPE_P (type)
|| DECIMAL_FLOAT_TYPE_P (itype))
? CONVERT_EXPR : NOP_EXPR, type, expr);
which as far as I can see doesn't do anything besides
exchanging CONVERT_EXPR for NOP_EXPR which is a noop to the IL.
So it appears this shouldn't be moved to match.pd at all?
It's also not a 1:1 move since you are changing 'expr'.
Thanks,
Richard.
> > Thanks,
> > Tamar
> >
> > Concretely it makes both these cases behave the same
> >
> > float e = (float)a * (float)b;
> > *c = (_Float16)e;
> >
> > and
> >
> > *c = (_Float16)((float)a * (float)b);
> >
> > Thanks,
> > Tamar
> >
> > gcc/ChangeLog:
> >
> > 2019-06-25 Tamar Christina
> >
> > * convert.c (convert_to_real_1): Move part of conversion code...
> > * match.pd: ...To here.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 2019-06-25 Tamar Christina
> >
> > * gcc.dg/type-convert-var.c: New test.
> >
> > --
>
--
Richard Biener
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany;
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg)
RE: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch)
Adding some maintainers > -Original Message- > From: gcc-patches-ow...@gcc.gnu.org On > Behalf Of Tamar Christina > Sent: Tuesday, June 25, 2019 09:31 > To: gcc-patches@gcc.gnu.org > Cc: nd > Subject: [GCC][middle-end] Add rules to strip away unneeded type casts > in expressions (2nd patch) > > Hi All, > > This is an updated version of my GCC-9 patch which moves part of the > type conversion code from convert.c to match.pd because match.pd is > able to apply this transformation in the presence of intermediate > temporary variables. > > The previous patch was only regtested on aarch64-none-linux-gnu and I > hadn't done a regression on x86_64-pc-linux-gnu only a bootstrap. The > previous patch was approved > > here https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00116.html > but before committing I ran a x86_64-pc-linux-gnu regtest to be sure > and this showed an issue with a DFP test. I Have fixed this by > removing the offending convert. > The convert was just saying "keep the type as is" but match.pd looped > here as it thinks the match did something and would try other > patterns, causing it to match itself again. > > Instead when there's nothing to update, I just don't do anything. > > The second change was to merge this with the existing pattern for > integer conversion in order to silence a warning from match.pd which > though that the two patterns overlaps because their match conditions > are similar (they have different conditions inside the ifs but > match.pd doesn't check those of course.). > > Regtested and bootstrapped on aarch64-none-linux-gnu and x86_64-pc- > linux-gnu and no issues. > > Ok for trunk? > > Thanks, > Tamar > > Concretely it makes both these cases behave the same > > float e = (float)a * (float)b; > *c = (_Float16)e; > > and > > *c = (_Float16)((float)a * (float)b); > > Thanks, > Tamar > > gcc/ChangeLog: > > 2019-06-25 Tamar Christina > > * convert.c (convert_to_real_1): Move part of conversion code... > * match.pd: ...To here. > > gcc/testsuite/ChangeLog: > > 2019-06-25 Tamar Christina > > * gcc.dg/type-convert-var.c: New test. > > --
[GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch)
Hi All,
This is an updated version of my GCC-9 patch which moves part of the type
conversion code
from convert.c to match.pd because match.pd is able to apply this
transformation in the
presence of intermediate temporary variables.
The previous patch was only regtested on aarch64-none-linux-gnu and I hadn't
done a
regression on x86_64-pc-linux-gnu only a bootstrap. The previous patch was
approved
here https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00116.html
but before committing I ran a x86_64-pc-linux-gnu regtest to be sure and this
showed an issue with a DFP test. I Have fixed this by removing the offending
convert.
The convert was just saying "keep the type as is" but match.pd looped here as
it thinks
the match did something and would try other patterns, causing it to match
itself again.
Instead when there's nothing to update, I just don't do anything.
The second change was to merge this with the existing pattern for integer
conversion
in order to silence a warning from match.pd which though that the two patterns
overlaps
because their match conditions are similar (they have different conditions
inside the ifs
but match.pd doesn't check those of course.).
Regtested and bootstrapped on aarch64-none-linux-gnu and x86_64-pc-linux-gnu
and no issues.
Ok for trunk?
Thanks,
Tamar
Concretely it makes both these cases behave the same
float e = (float)a * (float)b;
*c = (_Float16)e;
and
*c = (_Float16)((float)a * (float)b);
Thanks,
Tamar
gcc/ChangeLog:
2019-06-25 Tamar Christina
* convert.c (convert_to_real_1): Move part of conversion code...
* match.pd: ...To here.
gcc/testsuite/ChangeLog:
2019-06-25 Tamar Christina
* gcc.dg/type-convert-var.c: New test.
--
diff --git a/gcc/convert.c b/gcc/convert.c
index d5aa07b510e0e7831e8d121b383e42e5c6e89321..923eb70366e6c05141fb1580ba6f85e354aa3f76 100644
--- a/gcc/convert.c
+++ b/gcc/convert.c
@@ -298,92 +298,6 @@ convert_to_real_1 (tree type, tree expr, bool fold_p)
return build1 (TREE_CODE (expr), type, arg);
}
break;
- /* Convert (outertype)((innertype0)a+(innertype1)b)
- into ((newtype)a+(newtype)b) where newtype
- is the widest mode from all of these. */
- case PLUS_EXPR:
- case MINUS_EXPR:
- case MULT_EXPR:
- case RDIV_EXPR:
- {
- tree arg0 = strip_float_extensions (TREE_OPERAND (expr, 0));
- tree arg1 = strip_float_extensions (TREE_OPERAND (expr, 1));
-
- if (FLOAT_TYPE_P (TREE_TYPE (arg0))
- && FLOAT_TYPE_P (TREE_TYPE (arg1))
- && DECIMAL_FLOAT_TYPE_P (itype) == DECIMAL_FLOAT_TYPE_P (type))
- {
- tree newtype = type;
-
- if (TYPE_MODE (TREE_TYPE (arg0)) == SDmode
- || TYPE_MODE (TREE_TYPE (arg1)) == SDmode
- || TYPE_MODE (type) == SDmode)
- newtype = dfloat32_type_node;
- if (TYPE_MODE (TREE_TYPE (arg0)) == DDmode
- || TYPE_MODE (TREE_TYPE (arg1)) == DDmode
- || TYPE_MODE (type) == DDmode)
- newtype = dfloat64_type_node;
- if (TYPE_MODE (TREE_TYPE (arg0)) == TDmode
- || TYPE_MODE (TREE_TYPE (arg1)) == TDmode
- || TYPE_MODE (type) == TDmode)
-newtype = dfloat128_type_node;
- if (newtype == dfloat32_type_node
- || newtype == dfloat64_type_node
- || newtype == dfloat128_type_node)
- {
- expr = build2 (TREE_CODE (expr), newtype,
- convert_to_real_1 (newtype, arg0,
- fold_p),
- convert_to_real_1 (newtype, arg1,
- fold_p));
- if (newtype == type)
- return expr;
- break;
- }
-
- if (TYPE_PRECISION (TREE_TYPE (arg0)) > TYPE_PRECISION (newtype))
- newtype = TREE_TYPE (arg0);
- if (TYPE_PRECISION (TREE_TYPE (arg1)) > TYPE_PRECISION (newtype))
- newtype = TREE_TYPE (arg1);
- /* Sometimes this transformation is safe (cannot
- change results through affecting double rounding
- cases) and sometimes it is not. If NEWTYPE is
- wider than TYPE, e.g. (float)((long double)double
- + (long double)double) converted to
- (float)(double + double), the transformation is
- unsafe regardless of the details of the types
- involved; double rounding can arise if the result
- of NEWTYPE arithmetic is a NEWTYPE value half way
- between two representable TYPE values but the
- exact value is sufficiently different (in the
- right direction) for this difference to be
- visible in ITYPE arithmetic. If NEWTYPE is the
- same as TYPE, however, the transformation may be
- safe depending on the types involved: it is safe
- if the ITYPE has strictly more than twice as many
- mantissa bits as TYPE, can represent infinities
- and NaNs if the TYPE can, and has sufficient
- exponent range for the product or ratio of two
- values representable in the TYPE to be within the
- range of normal values of ITYPE. */
- if (TYPE_PRECISION (newtype) < TYPE_PRECISION (itype)
- &&
Re: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch)
Hi Marc,
> >>> + (nop:type (op (convert:ty1 @1) (convert:ty2 @2)
> >>
> >> Please don't use 'nop' directly, use 'convert' instead. This line is very
> >> suspicious, both arguments of op should have the same type. Specifying the
> >> outertype should be unnecessary, it is always 'type'. And if necessary, I
> >> expect '(convert:ty1 @1)' is the same as '{ arg0; }'.
> >>
> >
> > Ah I wasn't aware I could use arg0 here. I've updated the patch, though I
> > don't
> > really find this clearer.
>
> > + (convert (op (convert:ty1 { arg0; }) (convert:ty2 { arg1; })
>
> I think you misunderstood my point. What you wrote is equivalent to:
>
> (convert (op { arg0; } { arg1; }
>
> since arg0 already has type ty1. And I am complaining that both arguments
> to op must have the same type, but you are creating one of type ty1 and
> one of type ty2, which doesn't clearly indicate that ty1==ty2.
>
Ah ok, I've reverted the previous changes and added a types_match on ty1 and
ty2.
> Maybe experiment with
> (long double)some_float * (long double)some_double
> cast to either float or double.
>
I did, and none of them were an issue:
All of these worked fine and did the operation as expected as DFmode
now, whereas before all except the first would have used TFmode.
double foo (float a, double b)
{
return (long double)a * (long double)b;
}
double bar (float a, double b)
{
float x = (long double)a;
return x * (long double)b;
}
void foo_ (double b, double *c)
{
float a = (double)3.0d;
long double e = (long double)a * (long double)b;
*c = (double)e;
}
> SCALAR_FLOAT_TYPE_P may be safer than FLOAT_TYPE_P.
Hmm, I can't think of any reason why these rules shouldn't apply to vector or
complex float modes. The old code in convert.c already used FLOAT_TYPE_P.
I've attached the updated patch.
Thanks,
Tamar
>
> --
> Marc Glisse
--
diff --git a/gcc/convert.c b/gcc/convert.c
index 1a3353c870768a33fe22480ec97c7d3e0c504075..a16b7af0ec54693eb4f1e3a110aabc1aa18eb8df 100644
--- a/gcc/convert.c
+++ b/gcc/convert.c
@@ -295,92 +295,6 @@ convert_to_real_1 (tree type, tree expr, bool fold_p)
return build1 (TREE_CODE (expr), type, arg);
}
break;
- /* Convert (outertype)((innertype0)a+(innertype1)b)
- into ((newtype)a+(newtype)b) where newtype
- is the widest mode from all of these. */
- case PLUS_EXPR:
- case MINUS_EXPR:
- case MULT_EXPR:
- case RDIV_EXPR:
- {
- tree arg0 = strip_float_extensions (TREE_OPERAND (expr, 0));
- tree arg1 = strip_float_extensions (TREE_OPERAND (expr, 1));
-
- if (FLOAT_TYPE_P (TREE_TYPE (arg0))
- && FLOAT_TYPE_P (TREE_TYPE (arg1))
- && DECIMAL_FLOAT_TYPE_P (itype) == DECIMAL_FLOAT_TYPE_P (type))
- {
- tree newtype = type;
-
- if (TYPE_MODE (TREE_TYPE (arg0)) == SDmode
- || TYPE_MODE (TREE_TYPE (arg1)) == SDmode
- || TYPE_MODE (type) == SDmode)
- newtype = dfloat32_type_node;
- if (TYPE_MODE (TREE_TYPE (arg0)) == DDmode
- || TYPE_MODE (TREE_TYPE (arg1)) == DDmode
- || TYPE_MODE (type) == DDmode)
- newtype = dfloat64_type_node;
- if (TYPE_MODE (TREE_TYPE (arg0)) == TDmode
- || TYPE_MODE (TREE_TYPE (arg1)) == TDmode
- || TYPE_MODE (type) == TDmode)
-newtype = dfloat128_type_node;
- if (newtype == dfloat32_type_node
- || newtype == dfloat64_type_node
- || newtype == dfloat128_type_node)
- {
- expr = build2 (TREE_CODE (expr), newtype,
- convert_to_real_1 (newtype, arg0,
- fold_p),
- convert_to_real_1 (newtype, arg1,
- fold_p));
- if (newtype == type)
- return expr;
- break;
- }
-
- if (TYPE_PRECISION (TREE_TYPE (arg0)) > TYPE_PRECISION (newtype))
- newtype = TREE_TYPE (arg0);
- if (TYPE_PRECISION (TREE_TYPE (arg1)) > TYPE_PRECISION (newtype))
- newtype = TREE_TYPE (arg1);
- /* Sometimes this transformation is safe (cannot
- change results through affecting double rounding
- cases) and sometimes it is not. If NEWTYPE is
- wider than TYPE, e.g. (float)((long double)double
- + (long double)double) converted to
- (float)(double + double), the transformation is
- unsafe regardless of the details of the types
- involved; double rounding can arise if the result
- of NEWTYPE arithmetic is a NEWTYPE value half way
- between two representable TYPE values but the
- exact value is sufficiently different (in the
- right direction) for this difference to be
- visible in ITYPE arithmetic. If NEWTYPE is the
- same as TYPE, however, the transformation may be
- safe depending on the types involved: it is safe
- if the ITYPE has strictly more than twice as many
- mantissa bits as TYPE, can represent infinities
- and NaNs if the TYPE can, and has sufficient
- exponent range for the product or ratio of two
- values representable in the TYPE to be
Re: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch)
On Mon, 7 Jan 2019, Tamar Christina wrote:
The 01/04/2019 17:50, Marc Glisse wrote:
+(convert:newtype (op (convert:newtype @1) (convert:newtype @2)))
The outer 'convert' is unnecessary, op already has the same type.
Does it? The only comparison that has been done between the type of op and
"type" is that
they are both a decimal floating point type. I don't see any reason why they
have to be the
same type.
op is just PLUS_EXPR (or another operation), it isn't related to @0, it
does not have a type in itself. When you build the sum of 2 objects of
type newtype, the result has type newtype. On the other hand, if newtype
is not the same as type, you may be missing a conversion of the result to
type. Ah, I see that newtype is always == type here.
+ (nop:type (op (convert:ty1 @1) (convert:ty2 @2)
Please don't use 'nop' directly, use 'convert' instead. This line is very
suspicious, both arguments of op should have the same type. Specifying the
outertype should be unnecessary, it is always 'type'. And if necessary, I
expect '(convert:ty1 @1)' is the same as '{ arg0; }'.
Ah I wasn't aware I could use arg0 here. I've updated the patch, though I don't
really find this clearer.
+ (convert (op (convert:ty1 { arg0; }) (convert:ty2 { arg1; })
I think you misunderstood my point. What you wrote is equivalent to:
(convert (op { arg0; } { arg1; }
since arg0 already has type ty1. And I am complaining that both arguments
to op must have the same type, but you are creating one of type ty1 and
one of type ty2, which doesn't clearly indicate that ty1==ty2.
Maybe experiment with
(long double)some_float * (long double)some_double
cast to either float or double.
SCALAR_FLOAT_TYPE_P may be safer than FLOAT_TYPE_P.
--
Marc Glisse
Re: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch)
Hi Marc
The 01/04/2019 17:50, Marc Glisse wrote:
> > +(convert:newtype (op (convert:newtype @1) (convert:newtype @2)))
>
> The outer 'convert' is unnecessary, op already has the same type.
>
Does it? The only comparison that has been done between the type of op and
"type" is that
they are both a decimal floating point type. I don't see any reason why they
have to be the
same type.
> > + (nop:type (op (convert:ty1 @1) (convert:ty2 @2)
>
> Please don't use 'nop' directly, use 'convert' instead. This line is very
> suspicious, both arguments of op should have the same type. Specifying the
> outertype should be unnecessary, it is always 'type'. And if necessary, I
> expect '(convert:ty1 @1)' is the same as '{ arg0; }'.
>
Ah I wasn't aware I could use arg0 here. I've updated the patch, though I don't
really find this clearer.
Bootstrapped and regtested again on aarch64-none-linux-gnu and
x86_64-pc-linux-gnu and no issues.
Ok for trunk?
Thanks,
Tamar
> The explicit list of types is quite ugly, but since it already exists...
>
> --
> Marc Glisse
--
diff --git a/gcc/convert.c b/gcc/convert.c
index 1a3353c870768a33fe22480ec97c7d3e0c504075..a16b7af0ec54693eb4f1e3a110aabc1aa18eb8df 100644
--- a/gcc/convert.c
+++ b/gcc/convert.c
@@ -295,92 +295,6 @@ convert_to_real_1 (tree type, tree expr, bool fold_p)
return build1 (TREE_CODE (expr), type, arg);
}
break;
- /* Convert (outertype)((innertype0)a+(innertype1)b)
- into ((newtype)a+(newtype)b) where newtype
- is the widest mode from all of these. */
- case PLUS_EXPR:
- case MINUS_EXPR:
- case MULT_EXPR:
- case RDIV_EXPR:
- {
- tree arg0 = strip_float_extensions (TREE_OPERAND (expr, 0));
- tree arg1 = strip_float_extensions (TREE_OPERAND (expr, 1));
-
- if (FLOAT_TYPE_P (TREE_TYPE (arg0))
- && FLOAT_TYPE_P (TREE_TYPE (arg1))
- && DECIMAL_FLOAT_TYPE_P (itype) == DECIMAL_FLOAT_TYPE_P (type))
- {
- tree newtype = type;
-
- if (TYPE_MODE (TREE_TYPE (arg0)) == SDmode
- || TYPE_MODE (TREE_TYPE (arg1)) == SDmode
- || TYPE_MODE (type) == SDmode)
- newtype = dfloat32_type_node;
- if (TYPE_MODE (TREE_TYPE (arg0)) == DDmode
- || TYPE_MODE (TREE_TYPE (arg1)) == DDmode
- || TYPE_MODE (type) == DDmode)
- newtype = dfloat64_type_node;
- if (TYPE_MODE (TREE_TYPE (arg0)) == TDmode
- || TYPE_MODE (TREE_TYPE (arg1)) == TDmode
- || TYPE_MODE (type) == TDmode)
-newtype = dfloat128_type_node;
- if (newtype == dfloat32_type_node
- || newtype == dfloat64_type_node
- || newtype == dfloat128_type_node)
- {
- expr = build2 (TREE_CODE (expr), newtype,
- convert_to_real_1 (newtype, arg0,
- fold_p),
- convert_to_real_1 (newtype, arg1,
- fold_p));
- if (newtype == type)
- return expr;
- break;
- }
-
- if (TYPE_PRECISION (TREE_TYPE (arg0)) > TYPE_PRECISION (newtype))
- newtype = TREE_TYPE (arg0);
- if (TYPE_PRECISION (TREE_TYPE (arg1)) > TYPE_PRECISION (newtype))
- newtype = TREE_TYPE (arg1);
- /* Sometimes this transformation is safe (cannot
- change results through affecting double rounding
- cases) and sometimes it is not. If NEWTYPE is
- wider than TYPE, e.g. (float)((long double)double
- + (long double)double) converted to
- (float)(double + double), the transformation is
- unsafe regardless of the details of the types
- involved; double rounding can arise if the result
- of NEWTYPE arithmetic is a NEWTYPE value half way
- between two representable TYPE values but the
- exact value is sufficiently different (in the
- right direction) for this difference to be
- visible in ITYPE arithmetic. If NEWTYPE is the
- same as TYPE, however, the transformation may be
- safe depending on the types involved: it is safe
- if the ITYPE has strictly more than twice as many
- mantissa bits as TYPE, can represent infinities
- and NaNs if the TYPE can, and has sufficient
- exponent range for the product or ratio of two
- values representable in the TYPE to be within the
- range of normal values of ITYPE. */
- if (TYPE_PRECISION (newtype) < TYPE_PRECISION (itype)
- && (flag_unsafe_math_optimizations
- || (TYPE_PRECISION (newtype) == TYPE_PRECISION (type)
- && real_can_shorten_arithmetic (TYPE_MODE (itype),
- TYPE_MODE (type))
- && !excess_precision_type (newtype
- {
- expr = build2 (TREE_CODE (expr), newtype,
- convert_to_real_1 (newtype, arg0,
- fold_p),
- convert_to_real_1 (newtype, arg1,
- fold_p));
- if (newtype == type)
- return expr;
- }
- }
- }
- break;
default:
break;
}
diff --git a/gcc/match.pd b/gcc/match.pd
index 97a94cd8b2f2e0fee9ffbc76c5277c97689b6f42..ef4d9dcbc10d48b893ae5b7df06127a6dc51856a
Re: [GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch)
+(convert:newtype (op (convert:newtype @1) (convert:newtype @2)))
The outer 'convert' is unnecessary, op already has the same type.
+ (nop:type (op (convert:ty1 @1) (convert:ty2 @2)
Please don't use 'nop' directly, use 'convert' instead. This line is very
suspicious, both arguments of op should have the same type. Specifying the
outertype should be unnecessary, it is always 'type'. And if necessary, I
expect '(convert:ty1 @1)' is the same as '{ arg0; }'.
The explicit list of types is quite ugly, but since it already exists...
--
Marc Glisse
[GCC][middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch)
Hi All,
This is an updated version of my first patch which moves part of the type
conversion code
from convert.c to match.pd because match.pd is able to apply this
transformation in the
presence of intermediate temporary variables.
The previous patch was only regtested on aarch64-none-linux-gnu and I hadn't
done a
regression on x86_64-pc-linux-gnu only a bootstrap. The previous patch was
approved
here https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00116.html
but before committing I ran a x86_64-pc-linux-gnu regtest to be sure and this
showed an issue with a DFP test. I Have fixed this by removing the offending
convert.
The convert was just saying "keep the type as is" but match.pd looped here as
it thinks
the match did something and would try other patterns, causing it to match
itself again.
Instead when there's nothing to update, I just don't do anything.
The second change was to merge this with the existing pattern for integer
conversion
in order to silence a warning from match.pd which though that the two patterns
overlaps
because their match conditions are similar (they have different conditions
inside the ifs
but match.pd doesn't check those of course.).
Regtested and bootstrapped on aarch64-none-linux-gnu and x86_64-pc-linux-gnu
and no issues.
Ok for trunk?
Thanks,
Tamar
Concretely it makes both these cases behave the same
float e = (float)a * (float)b;
*c = (_Float16)e;
and
*c = (_Float16)((float)a * (float)b);
Thanks,
Tamar
gcc/ChangeLog:
2019-01-04 Tamar Christina
* convert.c (convert_to_real_1): Move part of conversion code...
* match.pd: ...To here.
gcc/testsuite/ChangeLog:
2019-01-04 Tamar Christina
* gcc.dg/type-convert-var.c: New test.
--
diff --git a/gcc/convert.c b/gcc/convert.c
index 1a3353c870768a33fe22480ec97c7d3e0c504075..a16b7af0ec54693eb4f1e3a110aabc1aa18eb8df 100644
--- a/gcc/convert.c
+++ b/gcc/convert.c
@@ -295,92 +295,6 @@ convert_to_real_1 (tree type, tree expr, bool fold_p)
return build1 (TREE_CODE (expr), type, arg);
}
break;
- /* Convert (outertype)((innertype0)a+(innertype1)b)
- into ((newtype)a+(newtype)b) where newtype
- is the widest mode from all of these. */
- case PLUS_EXPR:
- case MINUS_EXPR:
- case MULT_EXPR:
- case RDIV_EXPR:
- {
- tree arg0 = strip_float_extensions (TREE_OPERAND (expr, 0));
- tree arg1 = strip_float_extensions (TREE_OPERAND (expr, 1));
-
- if (FLOAT_TYPE_P (TREE_TYPE (arg0))
- && FLOAT_TYPE_P (TREE_TYPE (arg1))
- && DECIMAL_FLOAT_TYPE_P (itype) == DECIMAL_FLOAT_TYPE_P (type))
- {
- tree newtype = type;
-
- if (TYPE_MODE (TREE_TYPE (arg0)) == SDmode
- || TYPE_MODE (TREE_TYPE (arg1)) == SDmode
- || TYPE_MODE (type) == SDmode)
- newtype = dfloat32_type_node;
- if (TYPE_MODE (TREE_TYPE (arg0)) == DDmode
- || TYPE_MODE (TREE_TYPE (arg1)) == DDmode
- || TYPE_MODE (type) == DDmode)
- newtype = dfloat64_type_node;
- if (TYPE_MODE (TREE_TYPE (arg0)) == TDmode
- || TYPE_MODE (TREE_TYPE (arg1)) == TDmode
- || TYPE_MODE (type) == TDmode)
-newtype = dfloat128_type_node;
- if (newtype == dfloat32_type_node
- || newtype == dfloat64_type_node
- || newtype == dfloat128_type_node)
- {
- expr = build2 (TREE_CODE (expr), newtype,
- convert_to_real_1 (newtype, arg0,
- fold_p),
- convert_to_real_1 (newtype, arg1,
- fold_p));
- if (newtype == type)
- return expr;
- break;
- }
-
- if (TYPE_PRECISION (TREE_TYPE (arg0)) > TYPE_PRECISION (newtype))
- newtype = TREE_TYPE (arg0);
- if (TYPE_PRECISION (TREE_TYPE (arg1)) > TYPE_PRECISION (newtype))
- newtype = TREE_TYPE (arg1);
- /* Sometimes this transformation is safe (cannot
- change results through affecting double rounding
- cases) and sometimes it is not. If NEWTYPE is
- wider than TYPE, e.g. (float)((long double)double
- + (long double)double) converted to
- (float)(double + double), the transformation is
- unsafe regardless of the details of the types
- involved; double rounding can arise if the result
- of NEWTYPE arithmetic is a NEWTYPE value half way
- between two representable TYPE values but the
- exact value is sufficiently different (in the
- right direction) for this difference to be
- visible in ITYPE arithmetic. If NEWTYPE is the
- same as TYPE, however, the transformation may be
- safe depending on the types involved: it is safe
- if the ITYPE has strictly more than twice as many
- mantissa bits as TYPE, can represent infinities
- and NaNs if the TYPE can, and has sufficient
- exponent range for the product or ratio of two
- values representable in the TYPE to be within the
- range of normal values of ITYPE. */
- if (TYPE_PRECISION (newtype) < TYPE_PRECISION (itype)
- &&
