| Message ID | 878ux6khiu.fsf@talisman.default |
|---|---|
| State | New |
| Headers | show |
On 11/02/2013 10:25 AM, Richard Sandiford wrote: > Kenneth Zadeck <zadeck@naturalbridge.com> writes: >> On 11/02/2013 06:30 AM, Richard Sandiford wrote: >>> Bah. After all that effort, it turns out that -- by design -- >>> there is one special case where CONST_INTs are not sign-extended. >>> Nonzero/true BImode integers are stored as STORE_FLAG_VALUE, >>> which can be 1 rather than -1. So (const_int 1) can be a valid >>> BImode integer -- and consequently (const_int -1) can be wrong -- >>> even though BImode only has 1 bit. >>> >>> It might be nice to change that, but for wide-int I think we should >>> just treat rtxes like trees for now. >>> >>> Tested on powerpc64-linux-gnu and x86_64-linux-gnu. It fixes some ICEs >>> seen on bfin-elf. OK to install? >> do we have to throw away the baby with the bath water here? I guess >> what you are saying is that it is worse to have is_sign_extended be a >> variable that is almost always true than to be a hard false. > Right. is_sign_extended is only useful if it's a compile-time value. > Making it a run-time value would negate the benefit. > > I think in practice STORE_FLAG_VALUE is a compile-time constant too, > so we could set is_sign_extended to STORE_FLAG_VALUE == -1. But AFAICT > that would only help SPU and m68k. > >> also we could preserve the test and make it not apply to bimode. > You mean the one in the assert? Yeah, OK. How about this version? > > Richard > > > Index: gcc/rtl.h > =================================================================== > --- gcc/rtl.h 2013-11-02 11:06:12.738517644 +0000 > +++ gcc/rtl.h 2013-11-02 14:22:05.636007860 +0000 > @@ -1408,7 +1408,9 @@ typedef std::pair <rtx, enum machine_mod > { > static const enum precision_type precision_type = VAR_PRECISION; > static const bool host_dependent_precision = false; > - static const bool is_sign_extended = true; > + /* This ought to be true, except for the special case that BImode > + is canonicalized to STORE_FLAG_VALUE, which might be 1. */ > + static const bool is_sign_extended = false; > static unsigned int get_precision (const rtx_mode_t &); > static wi::storage_ref decompose (HOST_WIDE_INT *, unsigned int, > const rtx_mode_t &); > @@ -1432,7 +1434,8 @@ wi::int_traits <rtx_mode_t>::decompose ( > case CONST_INT: > if (precision < HOST_BITS_PER_WIDE_INT) > gcc_checking_assert (INTVAL (x.first) > - == sext_hwi (INTVAL (x.first), precision)); > + == sext_hwi (INTVAL (x.first), precision) > + || (precision == 1 && INTVAL (x.first) == 1)); > > return wi::storage_ref (&INTVAL (x.first), 1, precision); > yes, this is fine. kenny
On Sat, Nov 2, 2013 at 3:25 PM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Kenneth Zadeck <zadeck@naturalbridge.com> writes: >> On 11/02/2013 06:30 AM, Richard Sandiford wrote: >>> Bah. After all that effort, it turns out that -- by design -- >>> there is one special case where CONST_INTs are not sign-extended. >>> Nonzero/true BImode integers are stored as STORE_FLAG_VALUE, >>> which can be 1 rather than -1. So (const_int 1) can be a valid >>> BImode integer -- and consequently (const_int -1) can be wrong -- >>> even though BImode only has 1 bit. >>> >>> It might be nice to change that, but for wide-int I think we should >>> just treat rtxes like trees for now. >>> >>> Tested on powerpc64-linux-gnu and x86_64-linux-gnu. It fixes some ICEs >>> seen on bfin-elf. OK to install? >> do we have to throw away the baby with the bath water here? I guess >> what you are saying is that it is worse to have is_sign_extended be a >> variable that is almost always true than to be a hard false. > > Right. is_sign_extended is only useful if it's a compile-time value. > Making it a run-time value would negate the benefit. > > I think in practice STORE_FLAG_VALUE is a compile-time constant too, > so we could set is_sign_extended to STORE_FLAG_VALUE == -1. But AFAICT > that would only help SPU and m68k. > >> also we could preserve the test and make it not apply to bimode. > > You mean the one in the assert? Yeah, OK. How about this version? > > Richard > > > Index: gcc/rtl.h > =================================================================== > --- gcc/rtl.h 2013-11-02 11:06:12.738517644 +0000 > +++ gcc/rtl.h 2013-11-02 14:22:05.636007860 +0000 > @@ -1408,7 +1408,9 @@ typedef std::pair <rtx, enum machine_mod > { > static const enum precision_type precision_type = VAR_PRECISION; > static const bool host_dependent_precision = false; > - static const bool is_sign_extended = true; > + /* This ought to be true, except for the special case that BImode > + is canonicalized to STORE_FLAG_VALUE, which might be 1. */ > + static const bool is_sign_extended = false; > static unsigned int get_precision (const rtx_mode_t &); > static wi::storage_ref decompose (HOST_WIDE_INT *, unsigned int, > const rtx_mode_t &); > @@ -1432,7 +1434,8 @@ wi::int_traits <rtx_mode_t>::decompose ( > case CONST_INT: > if (precision < HOST_BITS_PER_WIDE_INT) > gcc_checking_assert (INTVAL (x.first) > - == sext_hwi (INTVAL (x.first), precision)); > + == sext_hwi (INTVAL (x.first), precision) > + || (precision == 1 && INTVAL (x.first) == 1)); please add a comment here (and a check for BImode?). Thanks, Richard. > return wi::storage_ref (&INTVAL (x.first), 1, precision); >
Index: gcc/rtl.h =================================================================== --- gcc/rtl.h 2013-11-02 11:06:12.738517644 +0000 +++ gcc/rtl.h 2013-11-02 14:22:05.636007860 +0000 @@ -1408,7 +1408,9 @@ typedef std::pair <rtx, enum machine_mod { static const enum precision_type precision_type = VAR_PRECISION; static const bool host_dependent_precision = false; - static const bool is_sign_extended = true; + /* This ought to be true, except for the special case that BImode + is canonicalized to STORE_FLAG_VALUE, which might be 1. */ + static const bool is_sign_extended = false; static unsigned int get_precision (const rtx_mode_t &); static wi::storage_ref decompose (HOST_WIDE_INT *, unsigned int, const rtx_mode_t &); @@ -1432,7 +1434,8 @@ wi::int_traits <rtx_mode_t>::decompose ( case CONST_INT: if (precision < HOST_BITS_PER_WIDE_INT) gcc_checking_assert (INTVAL (x.first) - == sext_hwi (INTVAL (x.first), precision)); + == sext_hwi (INTVAL (x.first), precision) + || (precision == 1 && INTVAL (x.first) == 1)); return wi::storage_ref (&INTVAL (x.first), 1, precision);