Patchwork Continue strict-volatile-bitfields fixes

login
register
mail settings
Submitter Bernd Schmidt
Date Jan. 23, 2012, 2:46 p.m.
Message ID <4F1D72CA.1060908@codesourcery.com>
Download mbox | patch
Permalink /patch/137368/
State New
Headers show

Comments

Bernd Schmidt - Jan. 23, 2012, 2:46 p.m.
On 11/29/2011 05:35 PM, Mitchell, Mark wrote:
>>> So, I still think this patch is the best way to go forward, and it
>> does
>>> fix incorrect code generation. Would appreciate an OK.
>>
>> Ping.
> 
> If you don't hear any objections within a week, please proceed.

That was committed a while ago. The part in stor-layout.c that stops us
from promoting bitfields to normal fields apparently caused some
testsuite regressions in sh tests, where some optimization dump scans
show that we can't perform the optimizations if there are BIT_FIELD_REFs
rather than a normal member access.

The testcases use things like
  enum something field:8;
and I think applying strict-volatile-bitfields to enums is probably
meaningless. Should we adjust semantics so that the compiler is free to
optimize enum bitfields? The patch would look something like the below.
Thomas has tested this on arm and sh with our internal tree.


Bernd


 		{
 		  DECL_ALIGN (decl) = MAX (xalign, DECL_ALIGN (decl));
 		  DECL_MODE (decl) = xmode;
DJ Delorie - Jan. 23, 2012, 7:51 p.m.
> and I think applying strict-volatile-bitfields to enums is probably
> meaningless.

MCU vendors would disagree.  If a location is volatile, it's most
likely hardware, and must be accessed in the user-specified way.
Randomly accessing adjacent locations could cause system failure.
Bernd Schmidt - Jan. 23, 2012, 10:08 p.m.
On 01/23/2012 08:51 PM, DJ Delorie wrote:
> 
>> and I think applying strict-volatile-bitfields to enums is probably
>> meaningless.
> 
> MCU vendors would disagree.  If a location is volatile, it's most
> likely hardware, and must be accessed in the user-specified way.
> Randomly accessing adjacent locations could cause system failure.

This is not an issue here. The optimization in question in stor-layout
tries to find out if a bitfield (volatile or not) is laid out in a way
that's equivalent to a natural machine mode. For example, a 2-byte
aligned 16 bit value can be HImode on most targets.

The earlier patch restricts this optimization for non-volatile bitfields
as well, on the grounds that an object may later be declared as volatile
even if the bitfield isn't declared volatile in the struct definition.

Strict-volatile-bitfields is concerned with the opposite case: ensuring
that an "int x:8;" is accessed only with int-sized accesses. My thought
was that this is probably pointless with enums, which I think aren't
usually thought of as a specific size unlike char/short/int. So, if we
see "enum something x:8;" we don't want to prevent the compiler from
treating this as a plain QImode non-bitfield member.


Bernd
Thomas Schwinge - Feb. 17, 2012, 8:51 p.m.
Hi!

How do we move this issue forward?

On Mon, 23 Jan 2012 15:46:34 +0100, Bernd Schmidt <bernds@codesourcery.com> wrote:
> On 11/29/2011 05:35 PM, Mitchell, Mark wrote:
> >>> So, I still think this patch is the best way to go forward, and it
> >> does
> >>> fix incorrect code generation. Would appreciate an OK.
> >>
> >> Ping.
> > 
> > If you don't hear any objections within a week, please proceed.
> 
> That was committed a while ago. The part in stor-layout.c that stops us
> from promoting bitfields to normal fields apparently caused some
> testsuite regressions in sh tests, where some optimization dump scans
> show that we can't perform the optimizations if there are BIT_FIELD_REFs
> rather than a normal member access.
> 
> The testcases use things like
>   enum something field:8;
> and I think applying strict-volatile-bitfields to enums is probably
> meaningless. Should we adjust semantics so that the compiler is free to
> optimize enum bitfields? The patch would look something like the below.
> Thomas has tested this on arm and sh with our internal tree.
> 
> 
> Bernd
> 
> 
> Index: gcc/stor-layout.c
> ===================================================================
> --- gcc/stor-layout.c	(revision 355696)
> +++ gcc/stor-layout.c	(working copy)
> @@ -665,8 +665,7 @@
>  	     may make a volatile object later.  */
>  	  if (TYPE_SIZE (type) != 0
>  	      && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST
> -	      && GET_MODE_CLASS (TYPE_MODE (type)) == MODE_INT
> -	      && flag_strict_volatile_bitfields <= 0)
> +	      && GET_MODE_CLASS (TYPE_MODE (type)) == MODE_INT)
>  	    {
>  	      enum machine_mode xmode
>  		= mode_for_size_tree (DECL_SIZE (decl), MODE_INT, 1);
> @@ -674,7 +673,12 @@
> 
>  	      if (xmode != BLKmode
>  		  && !(xalign > BITS_PER_UNIT && DECL_PACKED (decl))
> -		  && (known_align == 0 || known_align >= xalign))
> +		  && (known_align == 0 || known_align >= xalign)
> +                  && (flag_strict_volatile_bitfields <= 0
> +                      /* Same size makes strict volatile bitfields
> meaningless.  */
> +                      || GET_MODE_SIZE (xmode) == GET_MODE_SIZE
> (TYPE_MODE (type))
> +                      /* Strict volatile bitfields shouldn't apply to
> enums.  */
> +                      || TREE_CODE (type) == ENUMERAL_TYPE))
>  		{
>  		  DECL_ALIGN (decl) = MAX (xalign, DECL_ALIGN (decl));
>  		  DECL_MODE (decl) = xmode;
> 


Grüße,
 Thomas
Richard Guenther - Feb. 20, 2012, 11:14 a.m.
On Fri, Feb 17, 2012 at 9:51 PM, Thomas Schwinge
<thomas@codesourcery.com> wrote:
> Hi!
>
> How do we move this issue forward?
>
> On Mon, 23 Jan 2012 15:46:34 +0100, Bernd Schmidt <bernds@codesourcery.com> wrote:
>> On 11/29/2011 05:35 PM, Mitchell, Mark wrote:
>> >>> So, I still think this patch is the best way to go forward, and it
>> >> does
>> >>> fix incorrect code generation. Would appreciate an OK.
>> >>
>> >> Ping.
>> >
>> > If you don't hear any objections within a week, please proceed.
>>
>> That was committed a while ago. The part in stor-layout.c that stops us
>> from promoting bitfields to normal fields apparently caused some
>> testsuite regressions in sh tests, where some optimization dump scans
>> show that we can't perform the optimizations if there are BIT_FIELD_REFs
>> rather than a normal member access.
>>
>> The testcases use things like
>>   enum something field:8;
>> and I think applying strict-volatile-bitfields to enums is probably
>> meaningless. Should we adjust semantics so that the compiler is free to
>> optimize enum bitfields? The patch would look something like the below.
>> Thomas has tested this on arm and sh with our internal tree.
>>
>>
>> Bernd
>>
>>
>> Index: gcc/stor-layout.c
>> ===================================================================
>> --- gcc/stor-layout.c (revision 355696)
>> +++ gcc/stor-layout.c (working copy)
>> @@ -665,8 +665,7 @@
>>            may make a volatile object later.  */
>>         if (TYPE_SIZE (type) != 0
>>             && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST
>> -           && GET_MODE_CLASS (TYPE_MODE (type)) == MODE_INT
>> -           && flag_strict_volatile_bitfields <= 0)
>> +           && GET_MODE_CLASS (TYPE_MODE (type)) == MODE_INT)
>>           {
>>             enum machine_mode xmode
>>               = mode_for_size_tree (DECL_SIZE (decl), MODE_INT, 1);
>> @@ -674,7 +673,12 @@
>>
>>             if (xmode != BLKmode
>>                 && !(xalign > BITS_PER_UNIT && DECL_PACKED (decl))
>> -               && (known_align == 0 || known_align >= xalign))
>> +               && (known_align == 0 || known_align >= xalign)
>> +                  && (flag_strict_volatile_bitfields <= 0
>> +                      /* Same size makes strict volatile bitfields
>> meaningless.  */
>> +                      || GET_MODE_SIZE (xmode) == GET_MODE_SIZE
>> (TYPE_MODE (type))
>> +                      /* Strict volatile bitfields shouldn't apply to
>> enums.  */
>> +                      || TREE_CODE (type) == ENUMERAL_TYPE))

What about BOOLEAN_TYPE bitfields?  Thus, why not explicitely
spell out && TREE_CODE (type) == INTEGER_TYPE?

>>               {
>>                 DECL_ALIGN (decl) = MAX (xalign, DECL_ALIGN (decl));
>>                 DECL_MODE (decl) = xmode;
>>
>
>
> Grüße,
>  Thomas
Bernd Schmidt - Feb. 20, 2012, 5:28 p.m.
On 02/20/2012 12:14 PM, Richard Guenther wrote:
> On Fri, Feb 17, 2012 at 9:51 PM, Thomas Schwinge
> <thomas@codesourcery.com> wrote:
>> Hi!
>>
>> How do we move this issue forward?
>>
>> On Mon, 23 Jan 2012 15:46:34 +0100, Bernd Schmidt <bernds@codesourcery.com> wrote:
>>> That was committed a while ago. The part in stor-layout.c that stops us
>>> from promoting bitfields to normal fields apparently caused some
>>> testsuite regressions in sh tests, where some optimization dump scans
>>> show that we can't perform the optimizations if there are BIT_FIELD_REFs
>>> rather than a normal member access.
>>>
>>> The testcases use things like
>>>   enum something field:8;
>>> and I think applying strict-volatile-bitfields to enums is probably
>>> meaningless. Should we adjust semantics so that the compiler is free to
>>> optimize enum bitfields? The patch would look something like the below.

> 
> What about BOOLEAN_TYPE bitfields?  Thus, why not explicitely
> spell out && TREE_CODE (type) == INTEGER_TYPE?

That would work for me, if we can all agree that
-fstrict-volatile-bitfields should be restricted to INTEGER_TYPE.


Bernd
Richard Earnshaw - Feb. 20, 2012, 5:39 p.m.
On 20/02/12 17:28, Bernd Schmidt wrote:
> On 02/20/2012 12:14 PM, Richard Guenther wrote:
>> On Fri, Feb 17, 2012 at 9:51 PM, Thomas Schwinge
>> <thomas@codesourcery.com> wrote:
>>> Hi!
>>>
>>> How do we move this issue forward?
>>>
>>> On Mon, 23 Jan 2012 15:46:34 +0100, Bernd Schmidt <bernds@codesourcery.com> wrote:
>>>> That was committed a while ago. The part in stor-layout.c that stops us
>>>> from promoting bitfields to normal fields apparently caused some
>>>> testsuite regressions in sh tests, where some optimization dump scans
>>>> show that we can't perform the optimizations if there are BIT_FIELD_REFs
>>>> rather than a normal member access.
>>>>
>>>> The testcases use things like
>>>>   enum something field:8;
>>>> and I think applying strict-volatile-bitfields to enums is probably
>>>> meaningless. Should we adjust semantics so that the compiler is free to
>>>> optimize enum bitfields? The patch would look something like the below.
> 
>>
>> What about BOOLEAN_TYPE bitfields?  Thus, why not explicitely
>> spell out && TREE_CODE (type) == INTEGER_TYPE?
> 
> That would work for me, if we can all agree that
> -fstrict-volatile-bitfields should be restricted to INTEGER_TYPE.
> 
> 
> Bernd
> 


I'm not sure why it should be.  Can't a user write

#ifdef __cplusplus
#define BOOL bool
#else
#define bool _Bool
#endif

struct x {
  volatile BOOL a : 1;
  volatile BOOL b : 1;
  volatile unsigned char c : 6;
  volatile BOOL d : 1;
} y;

?

If you've got strict volatile bitfields, then the concept here is that
the access uses the declared type for accessing the member.  Since in
the ABI bool has a defined size, then it should access the member using
that size.

On ARM, sizeof bool is 1, so I'd take the above to mean that accessing
y.a to mean a read of a, b and c, but not d.


R.
Bernd Schmidt - Feb. 20, 2012, 5:51 p.m.
On 02/20/2012 06:39 PM, Richard Earnshaw wrote:
> I'm not sure why it should be.  Can't a user write
> 
> #ifdef __cplusplus
> #define BOOL bool
> #else
> #define bool _Bool
> #endif
> 
> struct x {
>   volatile BOOL a : 1;
>   volatile BOOL b : 1;
>   volatile unsigned char c : 6;
>   volatile BOOL d : 1;
> } y;
> 
> ?
> 
> If you've got strict volatile bitfields, then the concept here is that
> the access uses the declared type for accessing the member.  Since in
> the ABI bool has a defined size, then it should access the member using
> that size.
>
> On ARM, sizeof bool is 1, so I'd take the above to mean that accessing
> y.a to mean a read of a, b and c, but not d.

What are your thoughts on the argument about enums?


Bernd
Richard Earnshaw - Feb. 21, 2012, 9:40 a.m.
On 20/02/12 17:51, Bernd Schmidt wrote:
> On 02/20/2012 06:39 PM, Richard Earnshaw wrote:
>> I'm not sure why it should be.  Can't a user write
>>
>> #ifdef __cplusplus
>> #define BOOL bool
>> #else
>> #define bool _Bool
>> #endif
>>
>> struct x {
>>   volatile BOOL a : 1;
>>   volatile BOOL b : 1;
>>   volatile unsigned char c : 6;
>>   volatile BOOL d : 1;
>> } y;
>>
>> ?
>>
>> If you've got strict volatile bitfields, then the concept here is that
>> the access uses the declared type for accessing the member.  Since in
>> the ABI bool has a defined size, then it should access the member using
>> that size.
>>
>> On ARM, sizeof bool is 1, so I'd take the above to mean that accessing
>> y.a to mean a read of a, b and c, but not d.
> 
> What are your thoughts on the argument about enums?
> 
> 
> Bernd
> 

Similar.  A particular enumeration type has a defined size, so accesses
should use that size.

R.

Patch

Index: gcc/stor-layout.c
===================================================================
--- gcc/stor-layout.c	(revision 355696)
+++ gcc/stor-layout.c	(working copy)
@@ -665,8 +665,7 @@ 
 	     may make a volatile object later.  */
 	  if (TYPE_SIZE (type) != 0
 	      && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST
-	      && GET_MODE_CLASS (TYPE_MODE (type)) == MODE_INT
-	      && flag_strict_volatile_bitfields <= 0)
+	      && GET_MODE_CLASS (TYPE_MODE (type)) == MODE_INT)
 	    {
 	      enum machine_mode xmode
 		= mode_for_size_tree (DECL_SIZE (decl), MODE_INT, 1);
@@ -674,7 +673,12 @@ 

 	      if (xmode != BLKmode
 		  && !(xalign > BITS_PER_UNIT && DECL_PACKED (decl))
-		  && (known_align == 0 || known_align >= xalign))
+		  && (known_align == 0 || known_align >= xalign)
+                  && (flag_strict_volatile_bitfields <= 0
+                      /* Same size makes strict volatile bitfields
meaningless.  */
+                      || GET_MODE_SIZE (xmode) == GET_MODE_SIZE
(TYPE_MODE (type))
+                      /* Strict volatile bitfields shouldn't apply to
enums.  */
+                      || TREE_CODE (type) == ENUMERAL_TYPE))