[wide-int] Do not treat rtxes as sign-extended

Submitted by Richard Sandiford on Nov. 2, 2013, 2:25 p.m.

Details

Message ID 878ux6khiu.fsf@talisman.default
State New
Headers show

Commit Message

Richard Sandiford Nov. 2, 2013, 2:25 p.m.
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

Comments

Kenneth Zadeck Nov. 2, 2013, 2:48 p.m.
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
Richard Guenther Nov. 4, 2013, 11:23 a.m.
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);
>

Patch hide | download patch | download mbox

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);