| Message ID | 8737cjsbre.fsf@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, 5 May 2017, Richard Sandiford wrote: > Richard Biener <rguenther@suse.de> writes: > > On Fri, 5 May 2017, Georg-Johann Lay wrote: > >> On 05.05.2017 13:04, Richard Biener wrote: > >> > On Fri, 5 May 2017, Georg-Johann Lay wrote: > >> > > >> > > Applied this addendum to r247495 which removed flag_strict_overflow. There > >> > > were remains of the flag in avr.md which broke the avr build. > >> > > > >> > > Committed as r247632. > >> > > >> > Whoops - sorry for not grepping besides .[ch] files... > >> > > >> > But... these patterns very much look like premature optimization > >> > and/or bugs. combine is supposed to handle this via simplify_rtx. > >> > >> Well, for now the patch just restores avr BE to be able to be build. > > > > Sure. > > > >> > Also note that on RTL we generally assume overflow wraps as we lose > >> > signedness of operands. Not sure what 'compare' in your patterns > >> > will end up with. > >> > > >> > The only flag_wrapv checks in RTL otherwise are in simplify-rtx.c > >> > for ABS which seems to be a singed RTL op. > >> > >> Which is a bug, IMO. Letting undefined overflow propagate to RTL > >> renders some RTL as if it has undefined behaviour. Consequence is > >> that testing the MSB must no more use signed comparisons on > >> less-zero resp. greater-or-equal-to-zero. > >> > >> Cf. https://gcc.gnu.org/PR75964 for an example: > >> > >> > >> typedef __UINT8_TYPE__ uint8_t; > >> > >> uint8_t abs8 (uint8_t x) > >> { > >> if (x & 0x80) > >> x = -x; > >> > >> if (x & 0x80) > >> x = 0x7f; > >> > >> return x; > >> } > >> > >> The first comparison is performed by a signed test against 0 (which > >> is reasonable and the best code in that case) but then we conclude > >> that the second test is always false, which is BUG. > >> > >> IMO the culprit is to let slip undefined overflow to RTL. > > > > Yes. I thought in RTL overflow is always well-defined (but then > > as I said your patterns are equally bogus). > > Yeah, me too. I don't see how the simplify-rtx.c code can be right. > > Is the following OK, if it passes testing? Yes. Can you add the testcase? Thanks, Richard. > Thanks, > Richard > > > 2017-05-05 Richard Sandiford <richard.sandiford@linaro.org> > > gcc/ > PR rtl-optimization/75964 > * simplify-rtx.c (simplify_const_relational_operation): Remove > invalid handling of comparisons of integer ABS. > > Index: gcc/simplify-rtx.c > =================================================================== > --- gcc/simplify-rtx.c 2017-05-05 13:44:27.364724260 +0100 > +++ gcc/simplify-rtx.c 2017-05-05 13:44:36.580195277 +0100 > @@ -5316,34 +5316,14 @@ simplify_const_relational_operation (enu > { > case LT: > /* Optimize abs(x) < 0.0. */ > - if (!HONOR_SNANS (mode) > - && (!INTEGRAL_MODE_P (mode) > - || (!flag_wrapv && !flag_trapv))) > - { > - if (INTEGRAL_MODE_P (mode) > - && (issue_strict_overflow_warning > - (WARN_STRICT_OVERFLOW_CONDITIONAL))) > - warning (OPT_Wstrict_overflow, > - ("assuming signed overflow does not occur when " > - "assuming abs (x) < 0 is false")); > - return const0_rtx; > - } > + if (!INTEGRAL_MODE_P (mode) && !HONOR_SNANS (mode)) > + return const0_rtx; > break; > > case GE: > /* Optimize abs(x) >= 0.0. */ > - if (!HONOR_NANS (mode) > - && (!INTEGRAL_MODE_P (mode) > - || (!flag_wrapv && !flag_trapv))) > - { > - if (INTEGRAL_MODE_P (mode) > - && (issue_strict_overflow_warning > - (WARN_STRICT_OVERFLOW_CONDITIONAL))) > - warning (OPT_Wstrict_overflow, > - ("assuming signed overflow does not occur when " > - "assuming abs (x) >= 0 is true")); > - return const_true_rtx; > - } > + if (!INTEGRAL_MODE_P (mode) && !HONOR_NANS (mode)) > + return const_true_rtx; > break; > > case UNGE: > >
Index: gcc/simplify-rtx.c =================================================================== --- gcc/simplify-rtx.c 2017-05-05 13:44:27.364724260 +0100 +++ gcc/simplify-rtx.c 2017-05-05 13:44:36.580195277 +0100 @@ -5316,34 +5316,14 @@ simplify_const_relational_operation (enu { case LT: /* Optimize abs(x) < 0.0. */ - if (!HONOR_SNANS (mode) - && (!INTEGRAL_MODE_P (mode) - || (!flag_wrapv && !flag_trapv))) - { - if (INTEGRAL_MODE_P (mode) - && (issue_strict_overflow_warning - (WARN_STRICT_OVERFLOW_CONDITIONAL))) - warning (OPT_Wstrict_overflow, - ("assuming signed overflow does not occur when " - "assuming abs (x) < 0 is false")); - return const0_rtx; - } + if (!INTEGRAL_MODE_P (mode) && !HONOR_SNANS (mode)) + return const0_rtx; break; case GE: /* Optimize abs(x) >= 0.0. */ - if (!HONOR_NANS (mode) - && (!INTEGRAL_MODE_P (mode) - || (!flag_wrapv && !flag_trapv))) - { - if (INTEGRAL_MODE_P (mode) - && (issue_strict_overflow_warning - (WARN_STRICT_OVERFLOW_CONDITIONAL))) - warning (OPT_Wstrict_overflow, - ("assuming signed overflow does not occur when " - "assuming abs (x) >= 0 is true")); - return const_true_rtx; - } + if (!INTEGRAL_MODE_P (mode) && !HONOR_NANS (mode)) + return const_true_rtx; break; case UNGE: