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

login
register
mail settings
Submitter Richard Sandiford
Date Nov. 2, 2013, 2:25 p.m.
Message ID <878ux6khiu.fsf@talisman.default>
Download mbox | patch
Permalink /patch/288000/
State New
Headers show

Comments

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

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