| Message ID | 20190625083126.GA19632@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [middle-end] Add rules to strip away unneeded type casts in expressions (2nd patch) | expand |
Adding some maintainers > -----Original Message----- > From: gcc-patches-owner@gcc.gnu.org <gcc-patches-owner@gcc.gnu.org> On > Behalf Of Tamar Christina > Sent: Tuesday, June 25, 2019 09:31 > To: gcc-patches@gcc.gnu.org > Cc: nd <nd@arm.com> > 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 <tamar.christina@arm.com> > > * convert.c (convert_to_real_1): Move part of conversion code... > * match.pd: ...To here. > > gcc/testsuite/ChangeLog: > > 2019-06-25 Tamar Christina <tamar.christina@arm.com> > > * gcc.dg/type-convert-var.c: New test. > > --
On Tue, 25 Jun 2019, Tamar Christina wrote: > Adding some maintainers > > > -----Original Message----- > > From: gcc-patches-owner@gcc.gnu.org <gcc-patches-owner@gcc.gnu.org> On > > Behalf Of Tamar Christina > > Sent: Tuesday, June 25, 2019 09:31 > > To: gcc-patches@gcc.gnu.org > > Cc: nd <nd@arm.com> > > 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 <tamar.christina@arm.com> > > > > * convert.c (convert_to_real_1): Move part of conversion code... > > * match.pd: ...To here. > > > > gcc/testsuite/ChangeLog: > > > > 2019-06-25 Tamar Christina <tamar.christina@arm.com> > > > > * gcc.dg/type-convert-var.c: New test. > > > > -- >
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 <tamar.christina@arm.com> > > > > > > * convert.c (convert_to_real_1): Move part of conversion code... > > > * match.pd: ...To here. > > > > > > gcc/testsuite/ChangeLog: > > > > > > 2019-06-25 Tamar Christina <tamar.christina@arm.com> > > > > > > * gcc.dg/type-convert-var.c: New test. > > > > > > -- > > > > -- > Richard Biener <rguenther@suse.de> > SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg) --
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 <tamar.christina@arm.com> > > > > > > > > * convert.c (convert_to_real_1): Move part of conversion code... > > > > * match.pd: ...To here. > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > 2019-06-25 Tamar Christina <tamar.christina@arm.com> > > > > > > > > * gcc.dg/type-convert-var.c: New test. > > > > > > > > -- > > > > > > > -- > > Richard Biener <rguenther@suse.de> > > SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; > > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg) > >
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 <tamar.christina@arm.com> > > > > > > > > > > * convert.c (convert_to_real_1): Move part of conversion code... > > > > > * match.pd: ...To here. > > > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > > > 2019-06-25 Tamar Christina <tamar.christina@arm.com> > > > > > > > > > > * gcc.dg/type-convert-var.c: New test. > > > > > > > > > > -- > > > > > > > > > > -- > > > Richard Biener <rguenther@suse.de> > > > SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; > > > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg) > > > > > > -- > Richard Biener <rguenther@suse.de> > SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg) --
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. */ > > > > + (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 <tamar.christina@arm.com> > > > > > > > > > > > > * convert.c (convert_to_real_1): Move part of conversion code... > > > > > > * match.pd: ...To here. > > > > > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > > > > > 2019-06-25 Tamar Christina <tamar.christina@arm.com> > > > > > > > > > > > > * gcc.dg/type-convert-var.c: New test. > > > > > > > > > > > > -- > > > > > > > > > > > > > -- > > > > Richard Biener <rguenther@suse.de> > > > > SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; > > > > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg) > > > > > > > > > > -- > > Richard Biener <rguenther@suse.de> > > SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; > > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg) > >
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) - && (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 f9bc097c49122bf1b4bcf0b12b09840daf7b8fbc..228a69f99ae0318f034b2f713e522acd9e0995ae 100644 --- a/gcc/match.pd +++ b/gcc/match.pd @@ -4870,37 +4870,116 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) the C/C++ front-ends by shorten_binary_op and shorten_compare. Long term we want to move all that code out of the front-ends into here. */ -/* If we have a narrowing conversion of an arithmetic operation where - both operands are widening conversions from the same type as the outer - narrowing conversion. Then convert the innermost operands to a suitable - unsigned type (to avoid introducing undefined behavior), perform the - operation and convert the result to the desired type. */ -(for op (plus minus) - (simplify - (convert (op:s (convert@2 @0) (convert?@3 @1))) - (if (INTEGRAL_TYPE_P (type) - /* We check for type compatibility between @0 and @1 below, - so there's no need to check that @1/@3 are integral types. */ - && INTEGRAL_TYPE_P (TREE_TYPE (@0)) - && INTEGRAL_TYPE_P (TREE_TYPE (@2)) - /* The precision of the type of each operand must match the - precision of the mode of each operand, similarly for the - result. */ - && type_has_mode_precision_p (TREE_TYPE (@0)) - && type_has_mode_precision_p (TREE_TYPE (@1)) - && type_has_mode_precision_p (type) - /* The inner conversion must be a widening conversion. */ - && TYPE_PRECISION (TREE_TYPE (@2)) > TYPE_PRECISION (TREE_TYPE (@0)) - && types_match (@0, type) - && (types_match (@0, @1) - /* Or the second operand is const integer or converted const - integer from valueize. */ - || TREE_CODE (@1) == INTEGER_CST)) - (if (TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0))) - (op @0 (convert @1)) - (with { tree utype = unsigned_type_for (TREE_TYPE (@0)); } - (convert (op (convert:utype @0) - (convert:utype @1)))))))) +/* Convert (outertype)((innertype0)a+(innertype1)b) + into ((newtype)a+(newtype)b) where newtype + is the widest mode from all of these. */ +(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); + tree ty1 = TREE_TYPE (arg0); + tree ty2 = TREE_TYPE (arg1); + enum tree_code code = TREE_CODE (itype); } + (switch + (if (FLOAT_TYPE_P (ty1) + && FLOAT_TYPE_P (ty2) + && FLOAT_TYPE_P (type) + && DECIMAL_FLOAT_TYPE_P (itype) == DECIMAL_FLOAT_TYPE_P (type)) + (with { tree newtype = type; + if (TYPE_MODE (ty1) == SDmode + || TYPE_MODE (ty2) == SDmode + || TYPE_MODE (type) == SDmode) + newtype = dfloat32_type_node; + if (TYPE_MODE (ty1) == DDmode + || TYPE_MODE (ty2) == DDmode + || TYPE_MODE (type) == DDmode) + newtype = dfloat64_type_node; + if (TYPE_MODE (ty1) == TDmode + || TYPE_MODE (ty2) == TDmode + || TYPE_MODE (type) == TDmode) + newtype = dfloat128_type_node; } + (if ((newtype == dfloat32_type_node + || newtype == dfloat64_type_node + || newtype == dfloat128_type_node) + && newtype == type) + (convert:newtype (op (convert:newtype @1) (convert:newtype @2))) + (with { if (TYPE_PRECISION (ty1) > TYPE_PRECISION (newtype)) + newtype = ty1; + if (TYPE_PRECISION (ty2) > TYPE_PRECISION (newtype)) + newtype = ty2; } + /* 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)))) + (convert:newtype (op (convert:newtype @1) (convert:newtype @2))) + )))) ) + + (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))))) + + /* If we have a narrowing conversion of an arithmetic operation where + both operands are widening conversions from the same type as the outer + narrowing conversion. Then convert the innermost operands to a + suitable unsigned type (to avoid introducing undefined behavior), + perform the operation and convert the result to the desired type. */ + (if (INTEGRAL_TYPE_P (type) + && op != MULT_EXPR + && op != RDIV_EXPR + /* We check for type compatibility between @0 and @1 below, + so there's no need to check that @2/@4 are integral types. */ + && INTEGRAL_TYPE_P (TREE_TYPE (@1)) + && INTEGRAL_TYPE_P (TREE_TYPE (@3)) + /* The precision of the type of each operand must match the + precision of the mode of each operand, similarly for the + result. */ + && type_has_mode_precision_p (TREE_TYPE (@1)) + && type_has_mode_precision_p (TREE_TYPE (@2)) + && type_has_mode_precision_p (type) + /* The inner conversion must be a widening conversion. */ + && TYPE_PRECISION (TREE_TYPE (@3)) > TYPE_PRECISION (TREE_TYPE (@1)) + && types_match (@1, type) + && (types_match (@1, @2) + /* Or the second operand is const integer or converted const + integer from valueize. */ + || TREE_CODE (@2) == INTEGER_CST)) + (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) + (convert:utype @2)))))) + ) +))) /* This is another case of narrowing, specifically when there's an outer BIT_AND_EXPR which masks off bits outside the type of the innermost diff --git a/gcc/testsuite/gcc.dg/type-convert-var.c b/gcc/testsuite/gcc.dg/type-convert-var.c new file mode 100644 index 0000000000000000000000000000000000000000..88d74e2a49d7123515b87ff64a18bd9b306d57e9 --- /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" } } */