[PATCH] Use ATTRIBUTE_PACKED in ree.c

[PATCH] Use ATTRIBUTE_PACKED in ree.c

[Jakub Jelinek]

Hi!

The PR55430 miscompilation of ree.c by LRA lead me to look at
this structure, which was really meant to be just 2 byte long, but
is unnecessarily 4 byte long.  As it only contains 8, 2 and 1 bit bitfields,
it can always (except old alphas) be accessed by 1 byte loads/stores, so
packed attribute doesn't slow things down.  Fixed thusly, bootstrapped/regtested on
x86_64-linux and i686-linux, ok for trunk?

2012-11-21  Jakub Jelinek  <jakub@redhat.com>

	* ree.c (struct ext_modified): Add ATTRIBUTE_PACKED.

--- gcc/ree.c.jj	2012-11-21 16:00:13.000000000 +0100
+++ gcc/ree.c	2012-11-21 18:43:27.025706082 +0100
@@ -475,7 +475,7 @@ enum ext_modified_kind
   EXT_MODIFIED_SEXT
 };
 
-struct ext_modified
+struct ATTRIBUTE_PACKED ext_modified
 {
   /* Mode from which ree has zero or sign extended the destination.  */
   ENUM_BITFIELD(machine_mode) mode : 8;

	Jakub

Re: [PATCH] Use ATTRIBUTE_PACKED in ree.c

[Eric Botcazou]

> 2012-11-21  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* ree.c (struct ext_modified): Add ATTRIBUTE_PACKED.

Are you sure that this will compile with non-GCC compilers?

-- 
Eric Botcazou

Re: [PATCH] Use ATTRIBUTE_PACKED in ree.c

[Jakub Jelinek]

On Thu, Nov 22, 2012 at 12:26:01AM +0100, Eric Botcazou wrote:
> > 2012-11-21  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	* ree.c (struct ext_modified): Add ATTRIBUTE_PACKED.
> 
> Are you sure that this will compile with non-GCC compilers?

I'm not sure, but I hope it will.  ansidecl.h has
#ifndef GCC_VERSION
#define GCC_VERSION (__GNUC__ * 1000 + __GNUC_MINOR__)
#endif /* GCC_VERSION */
...
#if (GCC_VERSION < 2007)
# define __attribute__(x)
#endif
...
#ifndef ATTRIBUTE_PACKED
# define ATTRIBUTE_PACKED __attribute__ ((packed))
#endif

So in theory it should expand to nothing for non-GCC compilers.
I've tested (on a short testcase matching what the decl does)
GCCs from 4.8 down back to 3.2 (we support 4.1+ only anyway now)
and clang 3.0.

	Jakub

Re: [PATCH] Use ATTRIBUTE_PACKED in ree.c

[Eric Botcazou]

> I'm not sure, but I hope it will.  ansidecl.h has
> #ifndef GCC_VERSION
> #define GCC_VERSION (__GNUC__ * 1000 + __GNUC_MINOR__)
> #endif /* GCC_VERSION */
> ...
> #if (GCC_VERSION < 2007)
> # define __attribute__(x)
> #endif
> ...
> #ifndef ATTRIBUTE_PACKED
> # define ATTRIBUTE_PACKED __attribute__ ((packed))
> #endif
> 
> So in theory it should expand to nothing for non-GCC compilers.
> I've tested (on a short testcase matching what the decl does)
> GCCs from 4.8 down back to 3.2 (we support 4.1+ only anyway now)
> and clang 3.0.

OK, some other attributes are defined in the same way, thanks.

-- 
Eric Botcazou

Re: [PATCH] Use ATTRIBUTE_PACKED in ree.c

[Richard Earnshaw]

On 22/11/12 07:52, Jakub Jelinek wrote:
> On Thu, Nov 22, 2012 at 12:26:01AM +0100, Eric Botcazou wrote:
>>> 2012-11-21  Jakub Jelinek  <jakub@redhat.com>
>>>
>>> 	* ree.c (struct ext_modified): Add ATTRIBUTE_PACKED.
>>
>> Are you sure that this will compile with non-GCC compilers?
>
> I'm not sure, but I hope it will.  ansidecl.h has
> #ifndef GCC_VERSION
> #define GCC_VERSION (__GNUC__ * 1000 + __GNUC_MINOR__)
> #endif /* GCC_VERSION */
> ...
> #if (GCC_VERSION < 2007)
> # define __attribute__(x)
> #endif
> ...
> #ifndef ATTRIBUTE_PACKED
> # define ATTRIBUTE_PACKED __attribute__ ((packed))
> #endif
>
> So in theory it should expand to nothing for non-GCC compilers.
> I've tested (on a short testcase matching what the decl does)
> GCCs from 4.8 down back to 3.2 (we support 4.1+ only anyway now)
> and clang 3.0.
>
> 	Jakub
>

That's not going to help if those other compilers need packedness to 
eliminate padding.  The /old/ ARM ABI used to require that all structs 
were padded out to 32 bits.

It looks to me as though this code is just non-portable and as such 
needs to be rewritten :-(

R.

Re: [PATCH] Use ATTRIBUTE_PACKED in ree.c

[Jakub Jelinek]

On Wed, Nov 28, 2012 at 03:02:12PM +0000, Richard Earnshaw wrote:
> >#if (GCC_VERSION < 2007)
> ># define __attribute__(x)
> >#endif
> >...
> >#ifndef ATTRIBUTE_PACKED
> ># define ATTRIBUTE_PACKED __attribute__ ((packed))
> >#endif
> >
> >So in theory it should expand to nothing for non-GCC compilers.
> >I've tested (on a short testcase matching what the decl does)
> >GCCs from 4.8 down back to 3.2 (we support 4.1+ only anyway now)
> >and clang 3.0.
> >
> >	Jakub
> >
> 
> That's not going to help if those other compilers need packedness to
> eliminate padding.  The /old/ ARM ABI used to require that all
> structs were padded out to 32 bits.
> 
> It looks to me as though this code is just non-portable and as such
> needs to be rewritten :-(

Why?  There is no hard requirement that this must be 2 byte long instead of
4, it is a pure optimization, don't want to waste too much memory
unnecessarily.

	Jakub

Re: [PATCH] Use ATTRIBUTE_PACKED in ree.c

[Richard Earnshaw]

On 28/11/12 15:05, Jakub Jelinek wrote:
> On Wed, Nov 28, 2012 at 03:02:12PM +0000, Richard Earnshaw wrote:
>>> #if (GCC_VERSION < 2007)
>>> # define __attribute__(x)
>>> #endif
>>> ...
>>> #ifndef ATTRIBUTE_PACKED
>>> # define ATTRIBUTE_PACKED __attribute__ ((packed))
>>> #endif
>>>
>>> So in theory it should expand to nothing for non-GCC compilers.
>>> I've tested (on a short testcase matching what the decl does)
>>> GCCs from 4.8 down back to 3.2 (we support 4.1+ only anyway now)
>>> and clang 3.0.
>>>
>>> 	Jakub
>>>
>>
>> That's not going to help if those other compilers need packedness to
>> eliminate padding.  The /old/ ARM ABI used to require that all
>> structs were padded out to 32 bits.
>>
>> It looks to me as though this code is just non-portable and as such
>> needs to be rewritten :-(
>
> Why?  There is no hard requirement that this must be 2 byte long instead of
> 4, it is a pure optimization, don't want to waste too much memory
> unnecessarily.
>
> 	Jakub
>


Ah, OK.  I read your mail as implying that it didn't work when it wasn't 
2 bytes long...

R.