patch: honor volatile bitfield types

patch: honor volatile bitfield types

[DJ Delorie]

The saga continues.  Based on some private discussions, I've changed
the flag to be target-independent and added some i386-specific tests.
Thus, the functionality can be tested on all targets that have
memory-mapped peripheral registers (arm, sh, h8, m32c, rx, etc) as
well as verified on other targets.

I tried hacking gcc to make all structures volatile and running the
testsuite, but the output was full of complaints about parameter
mismatches and other problems caused just by the structs being
volatile.  It didn't totally blow up though :-)


	* common.opt (-fhonor-volatile-bitfield-types): new.
	* doc/invoke.texi: Document it.
	* fold-const.c (optimize_bit_field_compare): For volatile
	bitfields, use the field's type to determine the mode, not the
	field's size.
	* expr.c (expand_assignment): Likewise.
	(get_inner_reference): Likewise.
	(expand_expr_real_1): Likewise.
	* expmed.c (store_fixed_bit_field): Likewise.
	(extract_bit_field_1): Likewise.
	(extract_fixed_bit_field): Likewise.

	* gcc.target/i386/volatile-bitfields-1.c: New.
	* gcc.target/i386/volatile-bitfields-2.c: New.


Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 160589)
+++ doc/invoke.texi	(working copy)
@@ -17706,12 +17706,23 @@ be thrown between DSOs must be explicitl
 visibility so that the @samp{type_info} nodes will be unified between
 the DSOs.
 
 An overview of these techniques, their benefits and how to use them
 is at @w{@uref{http://gcc.gnu.org/wiki/Visibility}}.
 
+@item -fhonor-volatile-bitfield-types
+This option should be used if accesses to volatile bitfields (or other
+structure fields, although the compiler usually honors those types
+anyway) should use a single access in a mode of the same size as the
+container's type, aligned to a natural alignment if possible.  If the
+target requires strict alignment, and honoring the container type
+would require violating this alignment, a warning is issued.
+
+The default is to not honor the field's type, which results in a
+target-specific ``optimum'' access mode instead.
+
 @end table
 
 @c man end
 
 @node Environment Variables
 @section Environment Variables Affecting GCC
Index: fold-const.c
===================================================================
--- fold-const.c	(revision 160589)
+++ fold-const.c	(working copy)
@@ -3460,17 +3460,22 @@ optimize_bit_field_compare (location_t l
 	 || TREE_CODE (rinner) == PLACEHOLDER_EXPR)
        return 0;
    }
 
   /* See if we can find a mode to refer to this field.  We should be able to,
      but fail if we can't.  */
-  nmode = get_best_mode (lbitsize, lbitpos,
-			 const_p ? TYPE_ALIGN (TREE_TYPE (linner))
-			 : MIN (TYPE_ALIGN (TREE_TYPE (linner)),
-				TYPE_ALIGN (TREE_TYPE (rinner))),
-			 word_mode, lvolatilep || rvolatilep);
+  if (lvolatilep
+      && GET_MODE_BITSIZE (lmode) > 0
+      && flag_honor_volatile_bitfield_types)
+    nmode = lmode;
+  else
+    nmode = get_best_mode (lbitsize, lbitpos,
+			   const_p ? TYPE_ALIGN (TREE_TYPE (linner))
+			   : MIN (TYPE_ALIGN (TREE_TYPE (linner)),
+				  TYPE_ALIGN (TREE_TYPE (rinner))),
+			   word_mode, lvolatilep || rvolatilep);
   if (nmode == VOIDmode)
     return 0;
 
   /* Set signed and unsigned types of the precision of this mode for the
      shifts below.  */
   signed_type = lang_hooks.types.type_for_mode (nmode, 0);
Index: testsuite/gcc.target/i386/volatile-bitfields-2.c
===================================================================
--- testsuite/gcc.target/i386/volatile-bitfields-2.c	(revision 0)
+++ testsuite/gcc.target/i386/volatile-bitfields-2.c	(revision 0)
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-honor-volatile-bitfield-types" } */
+
+typedef struct {
+  char a:1;
+  char b:7;
+  int c;
+} BitStruct;
+
+volatile BitStruct bits;
+
+int foo ()
+{
+  return bits.b;
+}
+
+/* { dg-final { scan-assembler "movl.*bits" } } */
Index: testsuite/gcc.target/i386/volatile-bitfields-1.c
===================================================================
--- testsuite/gcc.target/i386/volatile-bitfields-1.c	(revision 0)
+++ testsuite/gcc.target/i386/volatile-bitfields-1.c	(revision 0)
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fhonor-volatile-bitfield-types" } */
+
+typedef struct {
+  char a:1;
+  char b:7;
+  int c;
+} BitStruct;
+
+volatile BitStruct bits;
+
+int foo ()
+{
+  return bits.b;
+}
+
+/* { dg-final { scan-assembler "movzbl.*bits" } } */
Index: expr.c
===================================================================
--- expr.c	(revision 160589)
+++ expr.c	(working copy)
@@ -4226,12 +4226,19 @@ expand_assignment (tree to, tree from, b
 
       /* If we are going to use store_bit_field and extract_bit_field,
 	 make sure to_rtx will be safe for multiple use.  */
 
       to_rtx = expand_normal (tem);
 
+      /* If the bitfield is volatile, we want to access it in the
+	 field's mode, not the computed mode.  */
+      if (volatilep
+	  && GET_CODE (to_rtx) == MEM
+	  && flag_honor_volatile_bitfield_types)
+	to_rtx = adjust_address (to_rtx, mode1, 0);
+ 
       if (offset != 0)
 	{
 	  enum machine_mode address_mode;
 	  rtx offset_rtx;
 
 	  if (!MEM_P (to_rtx))
@@ -5987,12 +5994,18 @@ get_inner_reference (tree exp, HOST_WIDE
       tree field = TREE_OPERAND (exp, 1);
       size_tree = DECL_SIZE (field);
       if (!DECL_BIT_FIELD (field))
 	mode = DECL_MODE (field);
       else if (DECL_MODE (field) == BLKmode)
 	blkmode_bitfield = true;
+      else if (TREE_THIS_VOLATILE (exp)
+	       && flag_honor_volatile_bitfield_types)
+	/* Volatile bitfields should be accessed in the mode of the
+	     field's type, not the mode computed based on the bit
+	     size.  */
+	mode = TYPE_MODE (DECL_BIT_FIELD_TYPE (field));
 
       *punsignedp = DECL_UNSIGNED (field);
     }
   else if (TREE_CODE (exp) == BIT_FIELD_REF)
     {
       size_tree = TREE_OPERAND (exp, 1);
@@ -8963,12 +8976,20 @@ expand_expr_real_1 (tree exp, rtx target
 			 VOIDmode,
 			 (modifier == EXPAND_INITIALIZER
 			  || modifier == EXPAND_CONST_ADDRESS
 			  || modifier == EXPAND_STACK_PARM)
 			 ? modifier : EXPAND_NORMAL);
 
+
+	/* If the bitfield is volatile, we want to access it in the
+	   field's mode, not the computed mode.  */
+	if (volatilep
+	    && GET_CODE (op0) == MEM
+	    && flag_honor_volatile_bitfield_types)
+	  op0 = adjust_address (op0, mode1, 0);
+
 	mode2
 	  = CONSTANT_P (op0) ? TYPE_MODE (TREE_TYPE (tem)) : GET_MODE (op0);
 
 	/* If we have either an offset, a BLKmode result, or a reference
 	   outside the underlying object, we must force it to memory.
 	   Such a case can occur in Ada if we have unchecked conversion
@@ -9088,12 +9109,15 @@ expand_expr_real_1 (tree exp, rtx target
 	    || REG_P (op0) || GET_CODE (op0) == SUBREG
 	    || (mode1 != BLKmode && ! direct_load[(int) mode1]
 		&& GET_MODE_CLASS (mode) != MODE_COMPLEX_INT
 		&& GET_MODE_CLASS (mode) != MODE_COMPLEX_FLOAT
 		&& modifier != EXPAND_CONST_ADDRESS
 		&& modifier != EXPAND_INITIALIZER)
+	    /* If the field is volatile, we always want an aligned
+	       access.  */
+	    || (volatilep && flag_honor_volatile_bitfield_types)
 	    /* If the field isn't aligned enough to fetch as a memref,
 	       fetch it as a bit field.  */
 	    || (mode1 != BLKmode
 		&& (((TYPE_ALIGN (TREE_TYPE (tem)) < GET_MODE_ALIGNMENT (mode)
 		      || (bitpos % GET_MODE_ALIGNMENT (mode) != 0)
 		      || (MEM_P (op0)
Index: expmed.c
===================================================================
--- expmed.c	(revision 160589)
+++ expmed.c	(working copy)
@@ -900,14 +900,20 @@ store_fixed_bit_field (rtx op0, unsigned
 	 We don't want a mode bigger than the destination.  */
 
       mode = GET_MODE (op0);
       if (GET_MODE_BITSIZE (mode) == 0
 	  || GET_MODE_BITSIZE (mode) > GET_MODE_BITSIZE (word_mode))
 	mode = word_mode;
-      mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
-			    MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0));
+
+      if (MEM_VOLATILE_P (op0)
+          && GET_MODE_BITSIZE (GET_MODE (op0)) > 0
+	  && flag_honor_volatile_bitfield_types)
+	mode = GET_MODE (op0);
+      else
+	mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
+			      MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0));
 
       if (mode == VOIDmode)
 	{
 	  /* The only way this should occur is if the field spans word
 	     boundaries.  */
 	  store_split_bit_field (op0, bitsize, bitpos + offset * BITS_PER_UNIT,
@@ -1374,12 +1380,20 @@ extract_bit_field_1 (rtx str_rtx, unsign
      we want a mode based on the size, so we must avoid calling it for FP
      modes.  */
   mode1  = (SCALAR_INT_MODE_P (tmode)
 	    ? mode_for_size (bitsize, GET_MODE_CLASS (tmode), 0)
 	    : mode);
 
+  /* If the bitfield is volatile, we need to make sure the access
+     remains on a type-aligned boundary.  */
+  if (GET_CODE (op0) == MEM
+      && MEM_VOLATILE_P (op0)
+      && GET_MODE_BITSIZE (GET_MODE (op0)) > 0
+      && flag_honor_volatile_bitfield_types)
+    goto no_subreg_mode_swap;
+
   if (((bitsize >= BITS_PER_WORD && bitsize == GET_MODE_BITSIZE (mode)
 	&& bitpos % BITS_PER_WORD == 0)
        || (mode1 != BLKmode
 	   /* ??? The big endian test here is wrong.  This is correct
 	      if the value is in a register, and if mode_for_size is not
 	      the same mode as op0.  This causes us to get unnecessarily
@@ -1726,14 +1740,25 @@ extract_fixed_bit_field (enum machine_mo
   else
     {
       /* Get the proper mode to use for this field.  We want a mode that
 	 includes the entire field.  If such a mode would be larger than
 	 a word, we won't be doing the extraction the normal way.  */
 
-      mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
-			    MEM_ALIGN (op0), word_mode, MEM_VOLATILE_P (op0));
+      if (MEM_VOLATILE_P (op0)
+	  && flag_honor_volatile_bitfield_types)
+	{
+	  if (GET_MODE_BITSIZE (GET_MODE (op0)) > 0)
+	    mode = GET_MODE (op0);
+	  else if (target && GET_MODE_BITSIZE (GET_MODE (target)) > 0)
+	    mode = GET_MODE (target);
+	  else
+	    mode = tmode;
+	}
+      else
+	mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
+			      MEM_ALIGN (op0), word_mode, MEM_VOLATILE_P (op0));
 
       if (mode == VOIDmode)
 	/* The only way this should occur is if the field spans word
 	   boundaries.  */
 	return extract_split_bit_field (op0, bitsize,
 					bitpos + offset * BITS_PER_UNIT,
@@ -1748,18 +1773,41 @@ extract_fixed_bit_field (enum machine_mo
 	{
 	  offset += (bitpos / total_bits) * (total_bits / BITS_PER_UNIT);
 	  bitpos -= ((bitpos / total_bits) * (total_bits / BITS_PER_UNIT)
 		     * BITS_PER_UNIT);
 	}
 
-      /* Get ref to an aligned byte, halfword, or word containing the field.
-	 Adjust BITPOS to be position within a word,
-	 and OFFSET to be the offset of that word.
-	 Then alter OP0 to refer to that word.  */
-      bitpos += (offset % (total_bits / BITS_PER_UNIT)) * BITS_PER_UNIT;
-      offset -= (offset % (total_bits / BITS_PER_UNIT));
+      /* If we're accessing a volatile MEM, we can't do the next
+	 alignment step if it results in a multi-word access where we
+	 otherwise wouldn't have one.  So, check for that case
+	 here.  */
+      if (MEM_P (op0)
+	  && MEM_VOLATILE_P (op0)
+	  && flag_honor_volatile_bitfield_types
+	  && bitpos + bitsize <= total_bits
+	  && bitpos + bitsize + (offset % (total_bits / BITS_PER_UNIT)) * BITS_PER_UNIT > total_bits)
+	{
+	  if (STRICT_ALIGNMENT)
+	    {
+	      if (bitsize == total_bits)
+		warning (0, "mis-aligned access required for structure member");
+	      else
+		warning (0, "mis-aligned access required for structure bitfield");
+	    }
+	}
+      else
+	{
+
+	  /* Get ref to an aligned byte, halfword, or word containing the field.
+	     Adjust BITPOS to be position within a word,
+	     and OFFSET to be the offset of that word.
+	     Then alter OP0 to refer to that word.  */
+	  bitpos += (offset % (total_bits / BITS_PER_UNIT)) * BITS_PER_UNIT;
+	  offset -= (offset % (total_bits / BITS_PER_UNIT));
+	}
+
       op0 = adjust_address (op0, mode, offset);
     }
 
   mode = GET_MODE (op0);
 
   if (BYTES_BIG_ENDIAN)
Index: common.opt
===================================================================
--- common.opt	(revision 160589)
+++ common.opt	(working copy)
@@ -622,12 +622,16 @@ Common Report Var(flag_loop_interchange)
 Enable Loop Interchange transformation
 
 floop-block
 Common Report Var(flag_loop_block) Optimization
 Enable Loop Blocking transformation
 
+fhonor-volatile-bitfield-types
+Common Report Var(flag_honor_volatile_bitfield_types)
+Force bitfield accesses to match their type width
+
 fguess-branch-probability
 Common Report Var(flag_guess_branch_prob) Optimization
 Enable guessing of branch probabilities
 
 ; Nonzero means ignore `#ident' directives.  0 means handle them.
 ; Generate position-independent code for executables if possible

Re: patch: honor volatile bitfield types

[Joseph S. Myers]

On Thu, 10 Jun 2010, DJ Delorie wrote:

> +The default is to not honor the field's type, which results in a
> +target-specific ``optimum'' access mode instead.

Shouldn't the default actually be target-specific (no new hooks needed, 
existing hooks to set options in a target-specific way should suffice, 
though maybe you need to initialize the flag setting to -1 to allow 
OVERRIDE_OPTIONS to know whether it was explicitly set)?  For example, ARM 
EABI should enable this by default.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: patch: honor volatile bitfield types

[DJ Delorie]

> Shouldn't the default actually be target-specific (no new hooks needed, 
> existing hooks to set options in a target-specific way should suffice, 
> though maybe you need to initialize the flag setting to -1 to allow 
> OVERRIDE_OPTIONS to know whether it was explicitly set)?  For example, ARM 
> EABI should enable this by default.

Yes, it should.  I figured, let's get the functionality working first,
then let the target maintainers decide which targets need it on and
which off.

Re: patch: honor volatile bitfield types

[Mark Mitchell]

DJ Delorie wrote:

>> Shouldn't the default actually be target-specific (no new hooks needed, 
>> existing hooks to set options in a target-specific way should suffice, 
>> though maybe you need to initialize the flag setting to -1 to allow 
>> OVERRIDE_OPTIONS to know whether it was explicitly set)?  For example, ARM 
>> EABI should enable this by default.
> 
> Yes, it should.  I figured, let's get the functionality working first,
> then let the target maintainers decide which targets need it on and
> which off.

Very glad to see this functionality.  However, before it goes in, I
think the change that Joseph is suggesting (which is perhaps just a
documentation change to say that the default is target-dependent) shold
occur.

Here's some proposed words for the documentation:

==
Access volatile structure fields (including bitfields) by using a single
read or write, and by loading as few bits as possible to load the field.
 For example, when loading a @type{char} bitfield on a machine with an
8-bit @type{char}, use either an 8-bit load instruction (if the entire
bitfield fits within a single byte) or a 16-bit load instruction (if the
bitfield spans two bytes).  If honoring these constraints would result
in an unaligned access on a machine where unaligned accesses are not
permitted, the compiler issues a warning and does not generate an
unaligned access.

If this option is disabled, the compiler will use the most efficient
instruction.  In the previous example, that might be a 32-bit load
instruction, even though that will access bytes that do not contain any
portion of the bitfield.

The default value of this option is determined by the application binary
interface for the target processor.
==

I think that's a more user-centric description of the option.  If I've
gotten the details wrong, please correct them.  If you need someone to
review this patch after it's been revised, please let me know.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: patch: honor volatile bitfield types

[Jeff Law]

On 06/11/10 10:15, Mark Mitchell wrote:
> DJ Delorie wrote:
>
>    
>>> Shouldn't the default actually be target-specific (no new hooks needed,
>>> existing hooks to set options in a target-specific way should suffice,
>>> though maybe you need to initialize the flag setting to -1 to allow
>>> OVERRIDE_OPTIONS to know whether it was explicitly set)?  For example, ARM
>>> EABI should enable this by default.
>>>        
>> Yes, it should.  I figured, let's get the functionality working first,
>> then let the target maintainers decide which targets need it on and
>> which off.
>>      
> Very glad to see this functionality.  However, before it goes in, I
> think the change that Joseph is suggesting (which is perhaps just a
> documentation change to say that the default is target-dependent) shold
> occur.
>    
Right.  Just to be clear, I asked DJ to make this a -f flag rather than 
a -m flag and that for targets where it should be the default (ARM EABI) 
the backend should arrange  for the flag to be turned on by default.

jeff

Re: patch: honor volatile bitfield types

[DJ Delorie]

Ok, updated documentation, and the default value for the flag is -1.
The checks are all "flag > 0" so it can be left at -1 or changed to
some other default, but the targets can see if it's still unspecified
and change it if they wish.

2010-06-11  DJ Delorie  <dj@redhat.com>

	* common.opt (-fhonor-volatile-bitfield-types): new.
	* doc/invoke.texi: Document it.
	* fold-const.c (optimize_bit_field_compare): For volatile
	bitfields, use the field's type to determine the mode, not the
	field's size.
	* expr.c (expand_assignment): Likewise.
	(get_inner_reference): Likewise.
	(expand_expr_real_1): Likewise.
	* expmed.c (store_fixed_bit_field): Likewise.
	(extract_bit_field_1): Likewise.
	(extract_fixed_bit_field): Likewise.

	* gcc.target/i386/volatile-bitfields-1.c: New.
	* gcc.target/i386/volatile-bitfields-2.c: New.

Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 160589)
+++ doc/invoke.texi	(working copy)
@@ -17706,12 +17706,38 @@ be thrown between DSOs must be explicitl
 visibility so that the @samp{type_info} nodes will be unified between
 the DSOs.
 
 An overview of these techniques, their benefits and how to use them
 is at @w{@uref{http://gcc.gnu.org/wiki/Visibility}}.
 
+@item -fhonor-volatile-bitfield-types
+This option should be used if accesses to volatile bitfields (or other
+structure fields, although the compiler usually honors those types
+anyway) should use a single access in a mode of the same size as the
+container's type, aligned to a natural alignment if possible.  For
+example, targets with memory-mapped peripheral registers might require
+all such accesses to be 16 bits wide; with this flag the user could
+declare all peripheral bitfields as ``unsigned short'' (assuming short
+is 16 bits on these targets) to force GCC to use 16 bit accesses
+instead of, perhaps, a more efficient 32 bit access.
+
+If this option is disabled, the compiler will use the most efficient
+instruction.  In the previous example, that might be a 32-bit load
+instruction, even though that will access bytes that do not contain
+any portion of the bitfield, or memory-mapped registers unrelated to
+the one being updated.
+
+If the target requires strict alignment, and honoring the container
+type would require violating this alignment, a warning is issued.
+However, the access happens as the user requested, under the
+assumption that the user knows something about the target hardware
+that GCC is unaware of.
+
+The default value of this option is determined by the application binary
+interface for the target processor.
+
 @end table
 
 @c man end
 
 @node Environment Variables
 @section Environment Variables Affecting GCC
Index: fold-const.c
===================================================================
--- fold-const.c	(revision 160589)
+++ fold-const.c	(working copy)
@@ -3460,17 +3460,22 @@ optimize_bit_field_compare (location_t l
 	 || TREE_CODE (rinner) == PLACEHOLDER_EXPR)
        return 0;
    }
 
   /* See if we can find a mode to refer to this field.  We should be able to,
      but fail if we can't.  */
-  nmode = get_best_mode (lbitsize, lbitpos,
-			 const_p ? TYPE_ALIGN (TREE_TYPE (linner))
-			 : MIN (TYPE_ALIGN (TREE_TYPE (linner)),
-				TYPE_ALIGN (TREE_TYPE (rinner))),
-			 word_mode, lvolatilep || rvolatilep);
+  if (lvolatilep
+      && GET_MODE_BITSIZE (lmode) > 0
+      && flag_honor_volatile_bitfield_types > 0)
+    nmode = lmode;
+  else
+    nmode = get_best_mode (lbitsize, lbitpos,
+			   const_p ? TYPE_ALIGN (TREE_TYPE (linner))
+			   : MIN (TYPE_ALIGN (TREE_TYPE (linner)),
+				  TYPE_ALIGN (TREE_TYPE (rinner))),
+			   word_mode, lvolatilep || rvolatilep);
   if (nmode == VOIDmode)
     return 0;
 
   /* Set signed and unsigned types of the precision of this mode for the
      shifts below.  */
   signed_type = lang_hooks.types.type_for_mode (nmode, 0);
Index: testsuite/gcc.target/i386/volatile-bitfields-2.c
===================================================================
--- testsuite/gcc.target/i386/volatile-bitfields-2.c	(revision 0)
+++ testsuite/gcc.target/i386/volatile-bitfields-2.c	(revision 0)
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-honor-volatile-bitfield-types" } */
+
+typedef struct {
+  char a:1;
+  char b:7;
+  int c;
+} BitStruct;
+
+volatile BitStruct bits;
+
+int foo ()
+{
+  return bits.b;
+}
+
+/* { dg-final { scan-assembler "movl.*bits" } } */
Index: testsuite/gcc.target/i386/volatile-bitfields-1.c
===================================================================
--- testsuite/gcc.target/i386/volatile-bitfields-1.c	(revision 0)
+++ testsuite/gcc.target/i386/volatile-bitfields-1.c	(revision 0)
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fhonor-volatile-bitfield-types" } */
+
+typedef struct {
+  char a:1;
+  char b:7;
+  int c;
+} BitStruct;
+
+volatile BitStruct bits;
+
+int foo ()
+{
+  return bits.b;
+}
+
+/* { dg-final { scan-assembler "movzbl.*bits" } } */
Index: expr.c
===================================================================
--- expr.c	(revision 160589)
+++ expr.c	(working copy)
@@ -4226,12 +4226,19 @@ expand_assignment (tree to, tree from, b
 
       /* If we are going to use store_bit_field and extract_bit_field,
 	 make sure to_rtx will be safe for multiple use.  */
 
       to_rtx = expand_normal (tem);
 
+      /* If the bitfield is volatile, we want to access it in the
+	 field's mode, not the computed mode.  */
+      if (volatilep
+	  && GET_CODE (to_rtx) == MEM
+	  && flag_honor_volatile_bitfield_types > 0)
+	to_rtx = adjust_address (to_rtx, mode1, 0);
+ 
       if (offset != 0)
 	{
 	  enum machine_mode address_mode;
 	  rtx offset_rtx;
 
 	  if (!MEM_P (to_rtx))
@@ -5987,12 +5994,18 @@ get_inner_reference (tree exp, HOST_WIDE
       tree field = TREE_OPERAND (exp, 1);
       size_tree = DECL_SIZE (field);
       if (!DECL_BIT_FIELD (field))
 	mode = DECL_MODE (field);
       else if (DECL_MODE (field) == BLKmode)
 	blkmode_bitfield = true;
+      else if (TREE_THIS_VOLATILE (exp)
+	       && flag_honor_volatile_bitfield_types > 0)
+	/* Volatile bitfields should be accessed in the mode of the
+	     field's type, not the mode computed based on the bit
+	     size.  */
+	mode = TYPE_MODE (DECL_BIT_FIELD_TYPE (field));
 
       *punsignedp = DECL_UNSIGNED (field);
     }
   else if (TREE_CODE (exp) == BIT_FIELD_REF)
     {
       size_tree = TREE_OPERAND (exp, 1);
@@ -8963,12 +8976,20 @@ expand_expr_real_1 (tree exp, rtx target
 			 VOIDmode,
 			 (modifier == EXPAND_INITIALIZER
 			  || modifier == EXPAND_CONST_ADDRESS
 			  || modifier == EXPAND_STACK_PARM)
 			 ? modifier : EXPAND_NORMAL);
 
+
+	/* If the bitfield is volatile, we want to access it in the
+	   field's mode, not the computed mode.  */
+	if (volatilep
+	    && GET_CODE (op0) == MEM
+	    && flag_honor_volatile_bitfield_types > 0)
+	  op0 = adjust_address (op0, mode1, 0);
+
 	mode2
 	  = CONSTANT_P (op0) ? TYPE_MODE (TREE_TYPE (tem)) : GET_MODE (op0);
 
 	/* If we have either an offset, a BLKmode result, or a reference
 	   outside the underlying object, we must force it to memory.
 	   Such a case can occur in Ada if we have unchecked conversion
@@ -9088,12 +9109,15 @@ expand_expr_real_1 (tree exp, rtx target
 	    || REG_P (op0) || GET_CODE (op0) == SUBREG
 	    || (mode1 != BLKmode && ! direct_load[(int) mode1]
 		&& GET_MODE_CLASS (mode) != MODE_COMPLEX_INT
 		&& GET_MODE_CLASS (mode) != MODE_COMPLEX_FLOAT
 		&& modifier != EXPAND_CONST_ADDRESS
 		&& modifier != EXPAND_INITIALIZER)
+	    /* If the field is volatile, we always want an aligned
+	       access.  */
+	    || (volatilep && flag_honor_volatile_bitfield_types > 0)
 	    /* If the field isn't aligned enough to fetch as a memref,
 	       fetch it as a bit field.  */
 	    || (mode1 != BLKmode
 		&& (((TYPE_ALIGN (TREE_TYPE (tem)) < GET_MODE_ALIGNMENT (mode)
 		      || (bitpos % GET_MODE_ALIGNMENT (mode) != 0)
 		      || (MEM_P (op0)
Index: expmed.c
===================================================================
--- expmed.c	(revision 160589)
+++ expmed.c	(working copy)
@@ -900,14 +900,20 @@ store_fixed_bit_field (rtx op0, unsigned
 	 We don't want a mode bigger than the destination.  */
 
       mode = GET_MODE (op0);
       if (GET_MODE_BITSIZE (mode) == 0
 	  || GET_MODE_BITSIZE (mode) > GET_MODE_BITSIZE (word_mode))
 	mode = word_mode;
-      mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
-			    MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0));
+
+      if (MEM_VOLATILE_P (op0)
+          && GET_MODE_BITSIZE (GET_MODE (op0)) > 0
+	  && flag_honor_volatile_bitfield_types > 0)
+	mode = GET_MODE (op0);
+      else
+	mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
+			      MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0));
 
       if (mode == VOIDmode)
 	{
 	  /* The only way this should occur is if the field spans word
 	     boundaries.  */
 	  store_split_bit_field (op0, bitsize, bitpos + offset * BITS_PER_UNIT,
@@ -1374,12 +1380,20 @@ extract_bit_field_1 (rtx str_rtx, unsign
      we want a mode based on the size, so we must avoid calling it for FP
      modes.  */
   mode1  = (SCALAR_INT_MODE_P (tmode)
 	    ? mode_for_size (bitsize, GET_MODE_CLASS (tmode), 0)
 	    : mode);
 
+  /* If the bitfield is volatile, we need to make sure the access
+     remains on a type-aligned boundary.  */
+  if (GET_CODE (op0) == MEM
+      && MEM_VOLATILE_P (op0)
+      && GET_MODE_BITSIZE (GET_MODE (op0)) > 0
+      && flag_honor_volatile_bitfield_types > 0)
+    goto no_subreg_mode_swap;
+
   if (((bitsize >= BITS_PER_WORD && bitsize == GET_MODE_BITSIZE (mode)
 	&& bitpos % BITS_PER_WORD == 0)
        || (mode1 != BLKmode
 	   /* ??? The big endian test here is wrong.  This is correct
 	      if the value is in a register, and if mode_for_size is not
 	      the same mode as op0.  This causes us to get unnecessarily
@@ -1726,14 +1740,25 @@ extract_fixed_bit_field (enum machine_mo
   else
     {
       /* Get the proper mode to use for this field.  We want a mode that
 	 includes the entire field.  If such a mode would be larger than
 	 a word, we won't be doing the extraction the normal way.  */
 
-      mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
-			    MEM_ALIGN (op0), word_mode, MEM_VOLATILE_P (op0));
+      if (MEM_VOLATILE_P (op0)
+	  && flag_honor_volatile_bitfield_types > 0)
+	{
+	  if (GET_MODE_BITSIZE (GET_MODE (op0)) > 0)
+	    mode = GET_MODE (op0);
+	  else if (target && GET_MODE_BITSIZE (GET_MODE (target)) > 0)
+	    mode = GET_MODE (target);
+	  else
+	    mode = tmode;
+	}
+      else
+	mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
+			      MEM_ALIGN (op0), word_mode, MEM_VOLATILE_P (op0));
 
       if (mode == VOIDmode)
 	/* The only way this should occur is if the field spans word
 	   boundaries.  */
 	return extract_split_bit_field (op0, bitsize,
 					bitpos + offset * BITS_PER_UNIT,
@@ -1748,18 +1773,41 @@ extract_fixed_bit_field (enum machine_mo
 	{
 	  offset += (bitpos / total_bits) * (total_bits / BITS_PER_UNIT);
 	  bitpos -= ((bitpos / total_bits) * (total_bits / BITS_PER_UNIT)
 		     * BITS_PER_UNIT);
 	}
 
-      /* Get ref to an aligned byte, halfword, or word containing the field.
-	 Adjust BITPOS to be position within a word,
-	 and OFFSET to be the offset of that word.
-	 Then alter OP0 to refer to that word.  */
-      bitpos += (offset % (total_bits / BITS_PER_UNIT)) * BITS_PER_UNIT;
-      offset -= (offset % (total_bits / BITS_PER_UNIT));
+      /* If we're accessing a volatile MEM, we can't do the next
+	 alignment step if it results in a multi-word access where we
+	 otherwise wouldn't have one.  So, check for that case
+	 here.  */
+      if (MEM_P (op0)
+	  && MEM_VOLATILE_P (op0)
+	  && flag_honor_volatile_bitfield_types > 0
+	  && bitpos + bitsize <= total_bits
+	  && bitpos + bitsize + (offset % (total_bits / BITS_PER_UNIT)) * BITS_PER_UNIT > total_bits)
+	{
+	  if (STRICT_ALIGNMENT)
+	    {
+	      if (bitsize == total_bits)
+		warning (0, "mis-aligned access required for structure member");
+	      else
+		warning (0, "mis-aligned access required for structure bitfield");
+	    }
+	}
+      else
+	{
+
+	  /* Get ref to an aligned byte, halfword, or word containing the field.
+	     Adjust BITPOS to be position within a word,
+	     and OFFSET to be the offset of that word.
+	     Then alter OP0 to refer to that word.  */
+	  bitpos += (offset % (total_bits / BITS_PER_UNIT)) * BITS_PER_UNIT;
+	  offset -= (offset % (total_bits / BITS_PER_UNIT));
+	}
+
       op0 = adjust_address (op0, mode, offset);
     }
 
   mode = GET_MODE (op0);
 
   if (BYTES_BIG_ENDIAN)
Index: common.opt
===================================================================
--- common.opt	(revision 160589)
+++ common.opt	(working copy)
@@ -622,12 +622,16 @@ Common Report Var(flag_loop_interchange)
 Enable Loop Interchange transformation
 
 floop-block
 Common Report Var(flag_loop_block) Optimization
 Enable Loop Blocking transformation
 
+fhonor-volatile-bitfield-types
+Common Report Var(flag_honor_volatile_bitfield_types) Init(-1)
+Force bitfield accesses to match their type width
+
 fguess-branch-probability
 Common Report Var(flag_guess_branch_prob) Optimization
 Enable guessing of branch probabilities
 
 ; Nonzero means ignore `#ident' directives.  0 means handle them.
 ; Generate position-independent code for executables if possible

Re: patch: honor volatile bitfield types

[Mark Mitchell]

DJ Delorie wrote:

> Ok, updated documentation, and the default value for the flag is -1.
> The checks are all "flag > 0" so it can be left at -1 or changed to
> some other default, but the targets can see if it's still unspecified
> and change it if they wish.

Thanks, both for that and for improving the documentation.

I'm still not very happy with the option name.  I don't think GCC
normally dishonors bitfield types. :-)  But, I guess I can't really
think of something I like a lot better.

I also don't think "strict alignment" is a very oft-used term outside of
GCC's internals.  I'd prefer to explicitly say what that means.

I think these warnings:

> +		warning (0, "mis-aligned access required for structure member");

should mention the fact that the bit-field is volatile and what the
consequences are.  Something like:

  using mis-aligned load to or store from field 'x' of 'struct a'
because multiple aligned loads/stores would be required and the field is
volatile

I think it really needs to be detailed at this level so that people have
some idea what's going on.  And, the field name should definitely be
mentioned if at all possible.

The patch is OK with changes in that spirit.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: patch: honor volatile bitfield types

[DJ Delorie]

> I'm still not very happy with the option name.

I'm open to suggestions :-)

> I don't think GCC normally dishonors bitfield types. :-)

The problem is, it *does* in that specific case.  Choosing an option
name that identifies the actual change in functionality without
confusing the user is tricky.

>   using mis-aligned load to or store from field 'x' of 'struct a'
> because multiple aligned loads/stores would be required and the field is
> volatile

That's not a warning, that's a paragraph :-)

Re: patch: honor volatile bitfield types

[Mark Mitchell]

DJ Delorie wrote:

>>   using mis-aligned load to or store from field 'x' of 'struct a'
>> because multiple aligned loads/stores would be required and the field is
>> volatile
> 
> That's not a warning, that's a paragraph :-)

This warning needs to be a paragraph; no phrase is going to be
meaningful to anyone that doesn't understand the compiler internals.
You can use the technique we use elsewhere where more explanation is
given the first time the warning is emitted.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: patch: honor volatile bitfield types

[Richard Guenther]

On Sat, Jun 12, 2010 at 3:15 AM, DJ Delorie <dj@redhat.com> wrote:
>
>> I'm still not very happy with the option name.
>
> I'm open to suggestions :-)

-fstrict-volatile-bitfields

>> I don't think GCC normally dishonors bitfield types. :-)
>
> The problem is, it *does* in that specific case.  Choosing an option
> name that identifies the actual change in functionality without
> confusing the user is tricky.
>
>>   using mis-aligned load to or store from field 'x' of 'struct a'
>> because multiple aligned loads/stores would be required and the field is
>> volatile
>
> That's not a warning, that's a paragraph :-)
>

Re: patch: honor volatile bitfield types

[DJ Delorie]

> And, the field name should definitely be mentioned if at all
> possible.

Sigh, not possible without major code changes.  This is the problem I
ran into writing the patch itself - the decls are all long since lost
by the time we get to these routines, and some of the paths that lead
to them make it very difficult to keep track of them (if they're
available at all).

How's this for the message?


	      if (bitsize == total_bits)
		warning (0, "mis-aligned access used for structure member");
	      else
		warning (0, "mis-aligned access used for structure bitfield");

	      if (! warned_about_misalignment)
		{
		  warned_about_misalignment = true;
		  warning (0, "volatile objects require a single access to preserve their"
			   " volatility, but this member spans multiple type-sized locations."
			   " Normally the compiler would use multiple accesses for such fields"
			   " to avoid mis-aligned accesses.  This code may fail at runtime")
		}

Re: patch: honor volatile bitfield types

[Mark Mitchell]

DJ Delorie wrote:
>> And, the field name should definitely be mentioned if at all
>> possible.
> 
> Sigh, not possible without major code changes. 

OK.

> 		  warning (0, "volatile objects require a single access to preserve their"
> 			   " volatility, but this member spans multiple type-sized locations."
> 			   " Normally the compiler would use multiple accesses for such fields"
> 			   " to avoid mis-aligned accesses.  This code may fail at runtime")

I think that can be made more precise.

use of 'volatile' objects should result in a single access to memory,
but this field cannot be accessed with a single aligned X-bit access.
Therefore, the compiler will use an unaligned access which may cause a
run-time fault.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: patch: honor volatile bitfield types

[DJ Delorie]

I like that.

What do you think of Richard's suggested option name?

	-fstrict-volatile-bitfields

Re: patch: honor volatile bitfield types

[Mark Mitchell]

DJ Delorie wrote:
> I like that.
> 
> What do you think of Richard's suggested option name?
> 
> 	-fstrict-volatile-bitfields

Works for me!

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: patch: honor volatile bitfield types

[Manuel López-Ibáñez]

On 15 June 2010 03:20, DJ Delorie <dj@redhat.com> wrote:
>
>> And, the field name should definitely be mentioned if at all
>> possible.
>
> Sigh, not possible without major code changes.  This is the problem I
> ran into writing the patch itself - the decls are all long since lost
> by the time we get to these routines, and some of the paths that lead
> to them make it very difficult to keep track of them (if they're
> available at all).
>
> How's this for the message?
>
>
>              if (bitsize == total_bits)
>                warning (0, "mis-aligned access used for structure member");
>              else
>                warning (0, "mis-aligned access used for structure bitfield");
>
>              if (! warned_about_misalignment)
>                {
>                  warned_about_misalignment = true;
>                  warning (0, "volatile objects require a single access to preserve their"
>                           " volatility, but this member spans multiple type-sized locations."
>                           " Normally the compiler would use multiple accesses for such fields"
>                           " to avoid mis-aligned accesses.  This code may fail at runtime")
>                }
>

You should use "inform" for such follow-up notes. Since the warning is
only enabled by fstrict-volatile-fields, you can use
OPT_fstrict_volatile_fields instead of 0. This way the option will be
shown alongside the warning.

I guess it is impossible to get a precise location here. I still would
prefer to use warning_at (input_location to remind us that this should
be fixed in the future.

Cheers,

Manuel.

Re: patch: honor volatile bitfield types

[DJ Delorie]

> Is this second message more approriate to "info" than "warning"?

Yes, but I wanted to keep them in sync wrt enabling/disabling
diagnostics by type.

Re: patch: honor volatile bitfield types

[Manuel López-Ibáñez]

On 15 June 2010 18:54, DJ Delorie <dj@redhat.com> wrote:
>
>> Is this second message more approriate to "info" than "warning"?
>
> Yes, but I wanted to keep them in sync wrt enabling/disabling
> diagnostics by type.

What does it mean that?

Enabling/disabling how?

Cheers,

Manuel.

Re: patch: honor volatile bitfield types

[Richard Henderson]

On 06/14/2010 06:20 PM, DJ Delorie wrote:
>> And, the field name should definitely be mentioned if at all
>> possible.
> 
> Sigh, not possible without major code changes.  This is the problem I
> ran into writing the patch itself - the decls are all long since lost
> by the time we get to these routines, and some of the paths that lead
> to them make it very difficult to keep track of them (if they're
> available at all).
> 
> How's this for the message?
> 
> 
> 	      if (bitsize == total_bits)
> 		warning (0, "mis-aligned access used for structure member");
> 	      else
> 		warning (0, "mis-aligned access used for structure bitfield");
> 
> 	      if (! warned_about_misalignment)
> 		{
> 		  warned_about_misalignment = true;
> 		  warning (0, "volatile objects require a single access to preserve their"
> 			   " volatility, but this member spans multiple type-sized locations."
> 			   " Normally the compiler would use multiple accesses for such fields"
> 			   " to avoid mis-aligned accesses.  This code may fail at runtime")
> 		}

Is this second message more approriate to "info" than "warning"?


r~

Re: patch: honor volatile bitfield types

[Richard Henderson]

On 06/15/2010 09:54 AM, DJ Delorie wrote:
>> Is this second message more approriate to "info" than "warning"?
> 
> Yes, but I wanted to keep them in sync wrt enabling/disabling
> diagnostics by type.

You mean e.g. "-w" disabling warnings?  I suppose that does 
require a bit of extra logic.  Though that would be easier
if warning returned a value indicating if the warning was
actually emitted...

r~

Re: patch: honor volatile bitfield types

[Manuel López-Ibáñez]

On 15 June 2010 21:11, Richard Henderson <rth@redhat.com> wrote:
> On 06/15/2010 09:54 AM, DJ Delorie wrote:
>>> Is this second message more approriate to "info" than "warning"?
>>
>> Yes, but I wanted to keep them in sync wrt enabling/disabling
>> diagnostics by type.
>
> You mean e.g. "-w" disabling warnings?  I suppose that does
> require a bit of extra logic.  Though that would be easier
> if warning returned a value indicating if the warning was
> actually emitted...

/* A warning at LOCATION.  Use this for code which is correct according to the
   relevant language specification but is likely to be buggy anyway.
   Returns true if the warning was printed, false if it was inhibited.  */

bool
warning_at (location_t location, int opt, const char *gmsgid, ...)


The same goes for pedwarn and permerror. This has been implemented
years ago for precisely this case.

Cheers,

Manuel.

Re: patch: honor volatile bitfield types

[DJ Delorie]

How about this, then?  New note wording, new option name.  Message
look like this (although not paragraph-wrapped):

dj.c: In function 'short_f':
dj.c:19:14: warning: mis-aligned access used for structure bitfield
dj.c:19:14: note: When a volatile object spans multiple type-sized
locations, the compiler must choose between using a single mis-aligned
access to preserve the volatility, or using multiple aligned accesses
to avoid runtime faults.  This code may fail at runtime if the
hardware does not allow this access.



	* common.opt (-fstrict-volatile-bitfields): new.
	* doc/invoke.texi: Document it.
	* fold-const.c (optimize_bit_field_compare): For volatile
	bitfields, use the field's type to determine the mode, not the
	field's size.
	* expr.c (expand_assignment): Likewise.
	(get_inner_reference): Likewise.
	(expand_expr_real_1): Likewise.
	* expmed.c (store_fixed_bit_field): Likewise.
	(extract_bit_field_1): Likewise.
	(extract_fixed_bit_field): Likewise.

	* gcc.target/i386/volatile-bitfields-1.c: New.
	* gcc.target/i386/volatile-bitfields-2.c: New.

Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 160810)
+++ doc/invoke.texi	(working copy)
@@ -17714,12 +17714,38 @@ be thrown between DSOs must be explicitl
 visibility so that the @samp{type_info} nodes will be unified between
 the DSOs.
 
 An overview of these techniques, their benefits and how to use them
 is at @w{@uref{http://gcc.gnu.org/wiki/Visibility}}.
 
+@item -fstrict-volatile-bitfields
+This option should be used if accesses to volatile bitfields (or other
+structure fields, although the compiler usually honors those types
+anyway) should use a single access in a mode of the same size as the
+container's type, aligned to a natural alignment if possible.  For
+example, targets with memory-mapped peripheral registers might require
+all such accesses to be 16 bits wide; with this flag the user could
+declare all peripheral bitfields as ``unsigned short'' (assuming short
+is 16 bits on these targets) to force GCC to use 16 bit accesses
+instead of, perhaps, a more efficient 32 bit access.
+
+If this option is disabled, the compiler will use the most efficient
+instruction.  In the previous example, that might be a 32-bit load
+instruction, even though that will access bytes that do not contain
+any portion of the bitfield, or memory-mapped registers unrelated to
+the one being updated.
+
+If the target requires strict alignment, and honoring the container
+type would require violating this alignment, a warning is issued.
+However, the access happens as the user requested, under the
+assumption that the user knows something about the target hardware
+that GCC is unaware of.
+
+The default value of this option is determined by the application binary
+interface for the target processor.
+
 @end table
 
 @c man end
 
 @node Environment Variables
 @section Environment Variables Affecting GCC
Index: fold-const.c
===================================================================
--- fold-const.c	(revision 160810)
+++ fold-const.c	(working copy)
@@ -3460,17 +3460,22 @@ optimize_bit_field_compare (location_t l
 	 || TREE_CODE (rinner) == PLACEHOLDER_EXPR)
        return 0;
    }
 
   /* See if we can find a mode to refer to this field.  We should be able to,
      but fail if we can't.  */
-  nmode = get_best_mode (lbitsize, lbitpos,
-			 const_p ? TYPE_ALIGN (TREE_TYPE (linner))
-			 : MIN (TYPE_ALIGN (TREE_TYPE (linner)),
-				TYPE_ALIGN (TREE_TYPE (rinner))),
-			 word_mode, lvolatilep || rvolatilep);
+  if (lvolatilep
+      && GET_MODE_BITSIZE (lmode) > 0
+      && flag_strict_volatile_bitfields > 0)
+    nmode = lmode;
+  else
+    nmode = get_best_mode (lbitsize, lbitpos,
+			   const_p ? TYPE_ALIGN (TREE_TYPE (linner))
+			   : MIN (TYPE_ALIGN (TREE_TYPE (linner)),
+				  TYPE_ALIGN (TREE_TYPE (rinner))),
+			   word_mode, lvolatilep || rvolatilep);
   if (nmode == VOIDmode)
     return 0;
 
   /* Set signed and unsigned types of the precision of this mode for the
      shifts below.  */
   signed_type = lang_hooks.types.type_for_mode (nmode, 0);
Index: testsuite/gcc.target/i386/volatile-bitfields-2.c
===================================================================
--- testsuite/gcc.target/i386/volatile-bitfields-2.c	(revision 0)
+++ testsuite/gcc.target/i386/volatile-bitfields-2.c	(revision 0)
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-volatile-bitfields" } */
+
+typedef struct {
+  char a:1;
+  char b:7;
+  int c;
+} BitStruct;
+
+volatile BitStruct bits;
+
+int foo ()
+{
+  return bits.b;
+}
+
+/* { dg-final { scan-assembler "movl.*bits" } } */
Index: testsuite/gcc.target/i386/volatile-bitfields-1.c
===================================================================
--- testsuite/gcc.target/i386/volatile-bitfields-1.c	(revision 0)
+++ testsuite/gcc.target/i386/volatile-bitfields-1.c	(revision 0)
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fstrict-volatile-bitfields" } */
+
+typedef struct {
+  char a:1;
+  char b:7;
+  int c;
+} BitStruct;
+
+volatile BitStruct bits;
+
+int foo ()
+{
+  return bits.b;
+}
+
+/* { dg-final { scan-assembler "movzbl.*bits" } } */
Index: expr.c
===================================================================
--- expr.c	(revision 160810)
+++ expr.c	(working copy)
@@ -4226,12 +4226,19 @@ expand_assignment (tree to, tree from, b
 
       /* If we are going to use store_bit_field and extract_bit_field,
 	 make sure to_rtx will be safe for multiple use.  */
 
       to_rtx = expand_normal (tem);
 
+      /* If the bitfield is volatile, we want to access it in the
+	 field's mode, not the computed mode.  */
+      if (volatilep
+	  && GET_CODE (to_rtx) == MEM
+	  && flag_strict_volatile_bitfields > 0)
+	to_rtx = adjust_address (to_rtx, mode1, 0);
+ 
       if (offset != 0)
 	{
 	  enum machine_mode address_mode;
 	  rtx offset_rtx;
 
 	  if (!MEM_P (to_rtx))
@@ -5987,12 +5994,18 @@ get_inner_reference (tree exp, HOST_WIDE
       tree field = TREE_OPERAND (exp, 1);
       size_tree = DECL_SIZE (field);
       if (!DECL_BIT_FIELD (field))
 	mode = DECL_MODE (field);
       else if (DECL_MODE (field) == BLKmode)
 	blkmode_bitfield = true;
+      else if (TREE_THIS_VOLATILE (exp)
+	       && flag_strict_volatile_bitfields > 0)
+	/* Volatile bitfields should be accessed in the mode of the
+	     field's type, not the mode computed based on the bit
+	     size.  */
+	mode = TYPE_MODE (DECL_BIT_FIELD_TYPE (field));
 
       *punsignedp = DECL_UNSIGNED (field);
     }
   else if (TREE_CODE (exp) == BIT_FIELD_REF)
     {
       size_tree = TREE_OPERAND (exp, 1);
@@ -8963,12 +8976,20 @@ expand_expr_real_1 (tree exp, rtx target
 			 VOIDmode,
 			 (modifier == EXPAND_INITIALIZER
 			  || modifier == EXPAND_CONST_ADDRESS
 			  || modifier == EXPAND_STACK_PARM)
 			 ? modifier : EXPAND_NORMAL);
 
+
+	/* If the bitfield is volatile, we want to access it in the
+	   field's mode, not the computed mode.  */
+	if (volatilep
+	    && GET_CODE (op0) == MEM
+	    && flag_strict_volatile_bitfields > 0)
+	  op0 = adjust_address (op0, mode1, 0);
+
 	mode2
 	  = CONSTANT_P (op0) ? TYPE_MODE (TREE_TYPE (tem)) : GET_MODE (op0);
 
 	/* If we have either an offset, a BLKmode result, or a reference
 	   outside the underlying object, we must force it to memory.
 	   Such a case can occur in Ada if we have unchecked conversion
@@ -9088,12 +9109,15 @@ expand_expr_real_1 (tree exp, rtx target
 	    || REG_P (op0) || GET_CODE (op0) == SUBREG
 	    || (mode1 != BLKmode && ! direct_load[(int) mode1]
 		&& GET_MODE_CLASS (mode) != MODE_COMPLEX_INT
 		&& GET_MODE_CLASS (mode) != MODE_COMPLEX_FLOAT
 		&& modifier != EXPAND_CONST_ADDRESS
 		&& modifier != EXPAND_INITIALIZER)
+	    /* If the field is volatile, we always want an aligned
+	       access.  */
+	    || (volatilep && flag_strict_volatile_bitfields > 0)
 	    /* If the field isn't aligned enough to fetch as a memref,
 	       fetch it as a bit field.  */
 	    || (mode1 != BLKmode
 		&& (((TYPE_ALIGN (TREE_TYPE (tem)) < GET_MODE_ALIGNMENT (mode)
 		      || (bitpos % GET_MODE_ALIGNMENT (mode) != 0)
 		      || (MEM_P (op0)
Index: expmed.c
===================================================================
--- expmed.c	(revision 160810)
+++ expmed.c	(working copy)
@@ -900,14 +900,20 @@ store_fixed_bit_field (rtx op0, unsigned
 	 We don't want a mode bigger than the destination.  */
 
       mode = GET_MODE (op0);
       if (GET_MODE_BITSIZE (mode) == 0
 	  || GET_MODE_BITSIZE (mode) > GET_MODE_BITSIZE (word_mode))
 	mode = word_mode;
-      mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
-			    MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0));
+
+      if (MEM_VOLATILE_P (op0)
+          && GET_MODE_BITSIZE (GET_MODE (op0)) > 0
+	  && flag_strict_volatile_bitfields > 0)
+	mode = GET_MODE (op0);
+      else
+	mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
+			      MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0));
 
       if (mode == VOIDmode)
 	{
 	  /* The only way this should occur is if the field spans word
 	     boundaries.  */
 	  store_split_bit_field (op0, bitsize, bitpos + offset * BITS_PER_UNIT,
@@ -1374,12 +1380,20 @@ extract_bit_field_1 (rtx str_rtx, unsign
      we want a mode based on the size, so we must avoid calling it for FP
      modes.  */
   mode1  = (SCALAR_INT_MODE_P (tmode)
 	    ? mode_for_size (bitsize, GET_MODE_CLASS (tmode), 0)
 	    : mode);
 
+  /* If the bitfield is volatile, we need to make sure the access
+     remains on a type-aligned boundary.  */
+  if (GET_CODE (op0) == MEM
+      && MEM_VOLATILE_P (op0)
+      && GET_MODE_BITSIZE (GET_MODE (op0)) > 0
+      && flag_strict_volatile_bitfields > 0)
+    goto no_subreg_mode_swap;
+
   if (((bitsize >= BITS_PER_WORD && bitsize == GET_MODE_BITSIZE (mode)
 	&& bitpos % BITS_PER_WORD == 0)
        || (mode1 != BLKmode
 	   /* ??? The big endian test here is wrong.  This is correct
 	      if the value is in a register, and if mode_for_size is not
 	      the same mode as op0.  This causes us to get unnecessarily
@@ -1726,14 +1740,25 @@ extract_fixed_bit_field (enum machine_mo
   else
     {
       /* Get the proper mode to use for this field.  We want a mode that
 	 includes the entire field.  If such a mode would be larger than
 	 a word, we won't be doing the extraction the normal way.  */
 
-      mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
-			    MEM_ALIGN (op0), word_mode, MEM_VOLATILE_P (op0));
+      if (MEM_VOLATILE_P (op0)
+	  && flag_strict_volatile_bitfields > 0)
+	{
+	  if (GET_MODE_BITSIZE (GET_MODE (op0)) > 0)
+	    mode = GET_MODE (op0);
+	  else if (target && GET_MODE_BITSIZE (GET_MODE (target)) > 0)
+	    mode = GET_MODE (target);
+	  else
+	    mode = tmode;
+	}
+      else
+	mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
+			      MEM_ALIGN (op0), word_mode, MEM_VOLATILE_P (op0));
 
       if (mode == VOIDmode)
 	/* The only way this should occur is if the field spans word
 	   boundaries.  */
 	return extract_split_bit_field (op0, bitsize,
 					bitpos + offset * BITS_PER_UNIT,
@@ -1748,18 +1773,55 @@ extract_fixed_bit_field (enum machine_mo
 	{
 	  offset += (bitpos / total_bits) * (total_bits / BITS_PER_UNIT);
 	  bitpos -= ((bitpos / total_bits) * (total_bits / BITS_PER_UNIT)
 		     * BITS_PER_UNIT);
 	}
 
-      /* Get ref to an aligned byte, halfword, or word containing the field.
-	 Adjust BITPOS to be position within a word,
-	 and OFFSET to be the offset of that word.
-	 Then alter OP0 to refer to that word.  */
-      bitpos += (offset % (total_bits / BITS_PER_UNIT)) * BITS_PER_UNIT;
-      offset -= (offset % (total_bits / BITS_PER_UNIT));
+      /* If we're accessing a volatile MEM, we can't do the next
+	 alignment step if it results in a multi-word access where we
+	 otherwise wouldn't have one.  So, check for that case
+	 here.  */
+      if (MEM_P (op0)
+	  && MEM_VOLATILE_P (op0)
+	  && flag_strict_volatile_bitfields > 0
+	  && bitpos + bitsize <= total_bits
+	  && bitpos + bitsize + (offset % (total_bits / BITS_PER_UNIT)) * BITS_PER_UNIT > total_bits)
+	{
+	  if (STRICT_ALIGNMENT)
+	    {
+	      static bool informed_about_misalignment = false;
+	      int warned;
+
+	      if (bitsize == total_bits)
+		warned = warning (0, "mis-aligned access used for structure member");
+	      else
+		warned = warning (0, "mis-aligned access used for structure bitfield");
+
+	      if (! informed_about_misalignment && warned)
+		{
+		  informed_about_misalignment = true;
+		  inform (input_location,
+			  "When a volatile object spans multiple type-sized locations,"
+			  " the compiler must choose between using a single mis-aligned access to"
+			  " preserve the volatility, or using multiple aligned accesses to avoid"
+			  " runtime faults.  This code may fail at runtime if the hardware does"
+			  " not allow this access.");
+		}
+	    }
+	}
+      else
+	{
+
+	  /* Get ref to an aligned byte, halfword, or word containing the field.
+	     Adjust BITPOS to be position within a word,
+	     and OFFSET to be the offset of that word.
+	     Then alter OP0 to refer to that word.  */
+	  bitpos += (offset % (total_bits / BITS_PER_UNIT)) * BITS_PER_UNIT;
+	  offset -= (offset % (total_bits / BITS_PER_UNIT));
+	}
+
       op0 = adjust_address (op0, mode, offset);
     }
 
   mode = GET_MODE (op0);
 
   if (BYTES_BIG_ENDIAN)
Index: common.opt
===================================================================
--- common.opt	(revision 160810)
+++ common.opt	(working copy)
@@ -626,12 +626,16 @@ Common Report Var(flag_loop_interchange)
 Enable Loop Interchange transformation
 
 floop-block
 Common Report Var(flag_loop_block) Optimization
 Enable Loop Blocking transformation
 
+fstrict-volatile-bitfields
+Common Report Var(flag_strict_volatile_bitfields) Init(-1)
+Force bitfield accesses to match their type width
+
 fguess-branch-probability
 Common Report Var(flag_guess_branch_prob) Optimization
 Enable guessing of branch probabilities
 
 ; Nonzero means ignore `#ident' directives.  0 means handle them.
 ; Generate position-independent code for executables if possible

Re: patch: honor volatile bitfield types

[Manuel López-Ibáñez]

On 16 June 2010 00:16, DJ Delorie <dj@redhat.com> wrote:
>
> How about this, then?  New note wording, new option name.  Message
> look like this (although not paragraph-wrapped):
> +             int warned;

Why not bool?

> +
> +             if (bitsize == total_bits)
> +               warned = warning (0, "mis-aligned access used for structure member");
> +             else
> +               warned = warning (0, "mis-aligned access used for structure bitfield");

I still think you should use warning_at (input_location,
OPT_fstrict_volatile_bitfields. If someone said already no, I haven't
seen that answer.

Cheers,

Manuel.

Re: patch: honor volatile bitfield types

[DJ Delorie]

> I still think you should use warning_at (input_location,
> OPT_fstrict_volatile_bitfields. If someone said already no, I
> haven't seen that answer.

There's no decl tree or insn rtl available at that point in gcc.  So,
I can't refer to the field or structure names, or the location of the
original field.  I wish I could - it would have made the patch logic a
lot simpler.

I put in the OPT_fstrict_volatile_bitfields though.

Re: patch: honor volatile bitfield types

[DJ Delorie]

> > +             int warned;
> 
> Why not bool?

No reason.  Fixed.

Re: patch: honor volatile bitfield types

[Mark Mitchell]

DJ Delorie wrote:
>> I still think you should use warning_at (input_location,
>> OPT_fstrict_volatile_bitfields. If someone said already no, I
>> haven't seen that answer.
> 
> There's no decl tree or insn rtl available at that point in gcc.  So,
> I can't refer to the field or structure names, or the location of the
> original field.  I wish I could - it would have made the patch logic a
> lot simpler.

This version of the patch is OK if there are no objections in 24 hours.

Thanks for sticking with it!

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: patch: honor volatile bitfield types

[Manuel López-Ibáñez]

On 16 June 2010 00:28, DJ Delorie <dj@redhat.com> wrote:
>
>> I still think you should use warning_at (input_location,
>> OPT_fstrict_volatile_bitfields. If someone said already no, I
>> haven't seen that answer.
>
> There's no decl tree or insn rtl available at that point in gcc.  So,
> I can't refer to the field or structure names, or the location of the
> original field.  I wish I could - it would have made the patch logic a
> lot simpler.

I don't mean the original location, just use input_location and
warning_at explicitly. That "warning" still exists is just a partial
transition that none has bothered to complete. The goal is to get rid
of input_location in diagnostic.c.

Cheers,

Manuel.

Re: patch: honor volatile bitfield types

[DJ Delorie]

> I don't mean the original location, just use input_location and
> warning_at explicitly.

Ok, changed.  There are still far more warning() than warning_at()
though, overall.

Re: patch: honor volatile bitfield types

[Mark Mitchell]

DJ Delorie wrote:
>> I don't mean the original location, just use input_location and
>> warning_at explicitly.
> 
> Ok, changed.  There are still far more warning() than warning_at()
> though, overall.

For avoidance of doubt, my approval applies just as well to these
subsequent minor improvements to this patch.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: patch: honor volatile bitfield types

[DJ Delorie]

Mark Mitchell <mark@codesourcery.com> writes:
> This version of the patch is OK if there are no objections in 24 hours.

Committed as attached.  Whew.

2010-06-16  DJ Delorie  <dj@redhat.com>

	* common.opt (-fstrict-volatile-bitfields): new.
	* doc/invoke.texi: Document it.
	* fold-const.c (optimize_bit_field_compare): For volatile
	bitfields, use the field's type to determine the mode, not the
	field's size.
	* expr.c (expand_assignment): Likewise.
	(get_inner_reference): Likewise.
	(expand_expr_real_1): Likewise.
	* expmed.c (store_fixed_bit_field): Likewise.
	(extract_bit_field_1): Likewise.
	(extract_fixed_bit_field): Likewise.

2010-06-16  DJ Delorie  <dj@redhat.com>

	* gcc.target/i386/volatile-bitfields-1.c: New.
	* gcc.target/i386/volatile-bitfields-2.c: New.

 
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 160864)
+++ gcc/doc/invoke.texi	(working copy)
@@ -17722,12 +17722,38 @@ be thrown between DSOs must be explicitl
 visibility so that the @samp{type_info} nodes will be unified between
 the DSOs.
 
 An overview of these techniques, their benefits and how to use them
 is at @w{@uref{http://gcc.gnu.org/wiki/Visibility}}.
 
+@item -fstrict-volatile-bitfields
+This option should be used if accesses to volatile bitfields (or other
+structure fields, although the compiler usually honors those types
+anyway) should use a single access in a mode of the same size as the
+container's type, aligned to a natural alignment if possible.  For
+example, targets with memory-mapped peripheral registers might require
+all such accesses to be 16 bits wide; with this flag the user could
+declare all peripheral bitfields as ``unsigned short'' (assuming short
+is 16 bits on these targets) to force GCC to use 16 bit accesses
+instead of, perhaps, a more efficient 32 bit access.
+
+If this option is disabled, the compiler will use the most efficient
+instruction.  In the previous example, that might be a 32-bit load
+instruction, even though that will access bytes that do not contain
+any portion of the bitfield, or memory-mapped registers unrelated to
+the one being updated.
+
+If the target requires strict alignment, and honoring the container
+type would require violating this alignment, a warning is issued.
+However, the access happens as the user requested, under the
+assumption that the user knows something about the target hardware
+that GCC is unaware of.
+
+The default value of this option is determined by the application binary
+interface for the target processor.
+
 @end table
 
 @c man end
 
 @node Environment Variables
 @section Environment Variables Affecting GCC
Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c	(revision 160864)
+++ gcc/fold-const.c	(working copy)
@@ -3460,17 +3460,22 @@ optimize_bit_field_compare (location_t l
 	 || TREE_CODE (rinner) == PLACEHOLDER_EXPR)
        return 0;
    }
 
   /* See if we can find a mode to refer to this field.  We should be able to,
      but fail if we can't.  */
-  nmode = get_best_mode (lbitsize, lbitpos,
-			 const_p ? TYPE_ALIGN (TREE_TYPE (linner))
-			 : MIN (TYPE_ALIGN (TREE_TYPE (linner)),
-				TYPE_ALIGN (TREE_TYPE (rinner))),
-			 word_mode, lvolatilep || rvolatilep);
+  if (lvolatilep
+      && GET_MODE_BITSIZE (lmode) > 0
+      && flag_strict_volatile_bitfields > 0)
+    nmode = lmode;
+  else
+    nmode = get_best_mode (lbitsize, lbitpos,
+			   const_p ? TYPE_ALIGN (TREE_TYPE (linner))
+			   : MIN (TYPE_ALIGN (TREE_TYPE (linner)),
+				  TYPE_ALIGN (TREE_TYPE (rinner))),
+			   word_mode, lvolatilep || rvolatilep);
   if (nmode == VOIDmode)
     return 0;
 
   /* Set signed and unsigned types of the precision of this mode for the
      shifts below.  */
   signed_type = lang_hooks.types.type_for_mode (nmode, 0);
Index: gcc/testsuite/gcc.target/i386/volatile-bitfields-2.c
===================================================================
--- gcc/testsuite/gcc.target/i386/volatile-bitfields-2.c	(revision 0)
+++ gcc/testsuite/gcc.target/i386/volatile-bitfields-2.c	(revision 0)
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-strict-volatile-bitfields" } */
+
+typedef struct {
+  char a:1;
+  char b:7;
+  int c;
+} BitStruct;
+
+volatile BitStruct bits;
+
+int foo ()
+{
+  return bits.b;
+}
+
+/* { dg-final { scan-assembler "movl.*bits" } } */
Index: gcc/testsuite/gcc.target/i386/volatile-bitfields-1.c
===================================================================
--- gcc/testsuite/gcc.target/i386/volatile-bitfields-1.c	(revision 0)
+++ gcc/testsuite/gcc.target/i386/volatile-bitfields-1.c	(revision 0)
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fstrict-volatile-bitfields" } */
+
+typedef struct {
+  char a:1;
+  char b:7;
+  int c;
+} BitStruct;
+
+volatile BitStruct bits;
+
+int foo ()
+{
+  return bits.b;
+}
+
+/* { dg-final { scan-assembler "movzbl.*bits" } } */
Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(revision 160864)
+++ gcc/expr.c	(working copy)
@@ -4226,12 +4226,19 @@ expand_assignment (tree to, tree from, b
 
       /* If we are going to use store_bit_field and extract_bit_field,
 	 make sure to_rtx will be safe for multiple use.  */
 
       to_rtx = expand_normal (tem);
 
+      /* If the bitfield is volatile, we want to access it in the
+	 field's mode, not the computed mode.  */
+      if (volatilep
+	  && GET_CODE (to_rtx) == MEM
+	  && flag_strict_volatile_bitfields > 0)
+	to_rtx = adjust_address (to_rtx, mode1, 0);
+ 
       if (offset != 0)
 	{
 	  enum machine_mode address_mode;
 	  rtx offset_rtx;
 
 	  if (!MEM_P (to_rtx))
@@ -5987,12 +5994,18 @@ get_inner_reference (tree exp, HOST_WIDE
       tree field = TREE_OPERAND (exp, 1);
       size_tree = DECL_SIZE (field);
       if (!DECL_BIT_FIELD (field))
 	mode = DECL_MODE (field);
       else if (DECL_MODE (field) == BLKmode)
 	blkmode_bitfield = true;
+      else if (TREE_THIS_VOLATILE (exp)
+	       && flag_strict_volatile_bitfields > 0)
+	/* Volatile bitfields should be accessed in the mode of the
+	     field's type, not the mode computed based on the bit
+	     size.  */
+	mode = TYPE_MODE (DECL_BIT_FIELD_TYPE (field));
 
       *punsignedp = DECL_UNSIGNED (field);
     }
   else if (TREE_CODE (exp) == BIT_FIELD_REF)
     {
       size_tree = TREE_OPERAND (exp, 1);
@@ -8963,12 +8976,20 @@ expand_expr_real_1 (tree exp, rtx target
 			 VOIDmode,
 			 (modifier == EXPAND_INITIALIZER
 			  || modifier == EXPAND_CONST_ADDRESS
 			  || modifier == EXPAND_STACK_PARM)
 			 ? modifier : EXPAND_NORMAL);
 
+
+	/* If the bitfield is volatile, we want to access it in the
+	   field's mode, not the computed mode.  */
+	if (volatilep
+	    && GET_CODE (op0) == MEM
+	    && flag_strict_volatile_bitfields > 0)
+	  op0 = adjust_address (op0, mode1, 0);
+
 	mode2
 	  = CONSTANT_P (op0) ? TYPE_MODE (TREE_TYPE (tem)) : GET_MODE (op0);
 
 	/* If we have either an offset, a BLKmode result, or a reference
 	   outside the underlying object, we must force it to memory.
 	   Such a case can occur in Ada if we have unchecked conversion
@@ -9088,12 +9109,15 @@ expand_expr_real_1 (tree exp, rtx target
 	    || REG_P (op0) || GET_CODE (op0) == SUBREG
 	    || (mode1 != BLKmode && ! direct_load[(int) mode1]
 		&& GET_MODE_CLASS (mode) != MODE_COMPLEX_INT
 		&& GET_MODE_CLASS (mode) != MODE_COMPLEX_FLOAT
 		&& modifier != EXPAND_CONST_ADDRESS
 		&& modifier != EXPAND_INITIALIZER)
+	    /* If the field is volatile, we always want an aligned
+	       access.  */
+	    || (volatilep && flag_strict_volatile_bitfields > 0)
 	    /* If the field isn't aligned enough to fetch as a memref,
 	       fetch it as a bit field.  */
 	    || (mode1 != BLKmode
 		&& (((TYPE_ALIGN (TREE_TYPE (tem)) < GET_MODE_ALIGNMENT (mode)
 		      || (bitpos % GET_MODE_ALIGNMENT (mode) != 0)
 		      || (MEM_P (op0)
Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c	(revision 160864)
+++ gcc/expmed.c	(working copy)
@@ -900,14 +900,20 @@ store_fixed_bit_field (rtx op0, unsigned
 	 We don't want a mode bigger than the destination.  */
 
       mode = GET_MODE (op0);
       if (GET_MODE_BITSIZE (mode) == 0
 	  || GET_MODE_BITSIZE (mode) > GET_MODE_BITSIZE (word_mode))
 	mode = word_mode;
-      mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
-			    MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0));
+
+      if (MEM_VOLATILE_P (op0)
+          && GET_MODE_BITSIZE (GET_MODE (op0)) > 0
+	  && flag_strict_volatile_bitfields > 0)
+	mode = GET_MODE (op0);
+      else
+	mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
+			      MEM_ALIGN (op0), mode, MEM_VOLATILE_P (op0));
 
       if (mode == VOIDmode)
 	{
 	  /* The only way this should occur is if the field spans word
 	     boundaries.  */
 	  store_split_bit_field (op0, bitsize, bitpos + offset * BITS_PER_UNIT,
@@ -1374,12 +1380,20 @@ extract_bit_field_1 (rtx str_rtx, unsign
      we want a mode based on the size, so we must avoid calling it for FP
      modes.  */
   mode1  = (SCALAR_INT_MODE_P (tmode)
 	    ? mode_for_size (bitsize, GET_MODE_CLASS (tmode), 0)
 	    : mode);
 
+  /* If the bitfield is volatile, we need to make sure the access
+     remains on a type-aligned boundary.  */
+  if (GET_CODE (op0) == MEM
+      && MEM_VOLATILE_P (op0)
+      && GET_MODE_BITSIZE (GET_MODE (op0)) > 0
+      && flag_strict_volatile_bitfields > 0)
+    goto no_subreg_mode_swap;
+
   if (((bitsize >= BITS_PER_WORD && bitsize == GET_MODE_BITSIZE (mode)
 	&& bitpos % BITS_PER_WORD == 0)
        || (mode1 != BLKmode
 	   /* ??? The big endian test here is wrong.  This is correct
 	      if the value is in a register, and if mode_for_size is not
 	      the same mode as op0.  This causes us to get unnecessarily
@@ -1726,14 +1740,25 @@ extract_fixed_bit_field (enum machine_mo
   else
     {
       /* Get the proper mode to use for this field.  We want a mode that
 	 includes the entire field.  If such a mode would be larger than
 	 a word, we won't be doing the extraction the normal way.  */
 
-      mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
-			    MEM_ALIGN (op0), word_mode, MEM_VOLATILE_P (op0));
+      if (MEM_VOLATILE_P (op0)
+	  && flag_strict_volatile_bitfields > 0)
+	{
+	  if (GET_MODE_BITSIZE (GET_MODE (op0)) > 0)
+	    mode = GET_MODE (op0);
+	  else if (target && GET_MODE_BITSIZE (GET_MODE (target)) > 0)
+	    mode = GET_MODE (target);
+	  else
+	    mode = tmode;
+	}
+      else
+	mode = get_best_mode (bitsize, bitpos + offset * BITS_PER_UNIT,
+			      MEM_ALIGN (op0), word_mode, MEM_VOLATILE_P (op0));
 
       if (mode == VOIDmode)
 	/* The only way this should occur is if the field spans word
 	   boundaries.  */
 	return extract_split_bit_field (op0, bitsize,
 					bitpos + offset * BITS_PER_UNIT,
@@ -1748,18 +1773,57 @@ extract_fixed_bit_field (enum machine_mo
 	{
 	  offset += (bitpos / total_bits) * (total_bits / BITS_PER_UNIT);
 	  bitpos -= ((bitpos / total_bits) * (total_bits / BITS_PER_UNIT)
 		     * BITS_PER_UNIT);
 	}
 
-      /* Get ref to an aligned byte, halfword, or word containing the field.
-	 Adjust BITPOS to be position within a word,
-	 and OFFSET to be the offset of that word.
-	 Then alter OP0 to refer to that word.  */
-      bitpos += (offset % (total_bits / BITS_PER_UNIT)) * BITS_PER_UNIT;
-      offset -= (offset % (total_bits / BITS_PER_UNIT));
+      /* If we're accessing a volatile MEM, we can't do the next
+	 alignment step if it results in a multi-word access where we
+	 otherwise wouldn't have one.  So, check for that case
+	 here.  */
+      if (MEM_P (op0)
+	  && MEM_VOLATILE_P (op0)
+	  && flag_strict_volatile_bitfields > 0
+	  && bitpos + bitsize <= total_bits
+	  && bitpos + bitsize + (offset % (total_bits / BITS_PER_UNIT)) * BITS_PER_UNIT > total_bits)
+	{
+	  if (STRICT_ALIGNMENT)
+	    {
+	      static bool informed_about_misalignment = false;
+	      bool warned;
+
+	      if (bitsize == total_bits)
+		warned = warning_at (input_location, OPT_fstrict_volatile_bitfields,
+				     "mis-aligned access used for structure member");
+	      else
+		warned = warning_at (input_location, OPT_fstrict_volatile_bitfields,
+				     "mis-aligned access used for structure bitfield");
+
+	      if (! informed_about_misalignment && warned)
+		{
+		  informed_about_misalignment = true;
+		  inform (input_location,
+			  "When a volatile object spans multiple type-sized locations,"
+			  " the compiler must choose between using a single mis-aligned access to"
+			  " preserve the volatility, or using multiple aligned accesses to avoid"
+			  " runtime faults.  This code may fail at runtime if the hardware does"
+			  " not allow this access.");
+		}
+	    }
+	}
+      else
+	{
+
+	  /* Get ref to an aligned byte, halfword, or word containing the field.
+	     Adjust BITPOS to be position within a word,
+	     and OFFSET to be the offset of that word.
+	     Then alter OP0 to refer to that word.  */
+	  bitpos += (offset % (total_bits / BITS_PER_UNIT)) * BITS_PER_UNIT;
+	  offset -= (offset % (total_bits / BITS_PER_UNIT));
+	}
+
       op0 = adjust_address (op0, mode, offset);
     }
 
   mode = GET_MODE (op0);
 
   if (BYTES_BIG_ENDIAN)
Index: gcc/common.opt
===================================================================
--- gcc/common.opt	(revision 160864)
+++ gcc/common.opt	(working copy)
@@ -626,12 +626,16 @@ Common Report Var(flag_loop_interchange)
 Enable Loop Interchange transformation
 
 floop-block
 Common Report Var(flag_loop_block) Optimization
 Enable Loop Blocking transformation
 
+fstrict-volatile-bitfields
+Common Report Var(flag_strict_volatile_bitfields) Init(-1)
+Force bitfield accesses to match their type width
+
 fguess-branch-probability
 Common Report Var(flag_guess_branch_prob) Optimization
 Enable guessing of branch probabilities
 
 ; Nonzero means ignore `#ident' directives.  0 means handle them.
 ; Generate position-independent code for executables if possible

Re: patch: honor volatile bitfield types

[Eric Botcazou]

> 2010-06-16  DJ Delorie  <dj@redhat.com>
>
> 	* gcc.target/i386/volatile-bitfields-1.c: New.

This one doesn't pass on i586:

foo:
        movb    bits, %al
        pushl   %ebp
        sarb    %al
        movl    %esp, %ebp
        movsbl  %al, %eax
        popl    %ebp
        ret

-- 
Eric Botcazou

Re: patch: honor volatile bitfield types

[DJ Delorie]

How about this?

/* { dg-final { scan-assembler "mov[zs]bl.*bits" } } */

Re: patch: honor volatile bitfield types

[Eric Botcazou]

> How about this?
>
> /* { dg-final { scan-assembler "mov[zs]bl.*bits" } } */

This doesn't match the changed line, does it?

-- 
Eric Botcazou

Re: patch: honor volatile bitfield types

[DJ Delorie]

doh, you're right.  I was focusing on the movsbl and not the operands

/* { dg-final { scan-assembler "(movb|mov[zs]bl).*bits" } } */

Are there any other ways to load a byte than mobv and mov[sz]bl ?

Re: patch: honor volatile bitfield types

[Eric Botcazou]

> doh, you're right.  I was focusing on the movsbl and not the operands
>
> /* { dg-final { scan-assembler "(movb|mov[zs]bl).*bits" } } */
>
> Are there any other ways to load a byte than mobv and mov[sz]bl ?

In my experience, there are only 2 variants of generated code in this area, 
one for i[345]86 and one for i686+, so that's probably good enough.

-- 
Eric Botcazou

Re: patch: honor volatile bitfield types

[DJ Delorie]

Can't use [] in dg-* as it looks like a TCL command :-P

This passes i686- and i586-pc-linux.  Ok?


	* gcc.target/i386/volatile-bitfields-1.c: Fix regex to match more
	correct opcodes.

Index: gcc.target/i386/volatile-bitfields-1.c
===================================================================
--- gcc.target/i386/volatile-bitfields-1.c	(revision 161225)
+++ gcc.target/i386/volatile-bitfields-1.c	(working copy)
@@ -11,7 +11,7 @@ volatile BitStruct bits;
 
 int foo ()
 {
   return bits.b;
 }
 
-/* { dg-final { scan-assembler "movzbl.*bits" } } */
+/* { dg-final { scan-assembler "(movsbl|movzbl|movb).*bits" } } */

Re: patch: honor volatile bitfield types

[Jie Zhang]

Hi DJ,

On 06/17/2010 06:53 AM, DJ Delorie wrote:
> Committed as attached.  Whew.
>
> 2010-06-16  DJ Delorie<dj@redhat.com>
>
> 	* common.opt (-fstrict-volatile-bitfields): new.
> 	* doc/invoke.texi: Document it.
> 	* fold-const.c (optimize_bit_field_compare): For volatile
> 	bitfields, use the field's type to determine the mode, not the
> 	field's size.
> 	* expr.c (expand_assignment): Likewise.
> 	(get_inner_reference): Likewise.
> 	(expand_expr_real_1): Likewise.
> 	* expmed.c (store_fixed_bit_field): Likewise.
> 	(extract_bit_field_1): Likewise.
> 	(extract_fixed_bit_field): Likewise.
>
> 2010-06-16  DJ Delorie<dj@redhat.com>
>
> 	* gcc.target/i386/volatile-bitfields-1.c: New.
> 	* gcc.target/i386/volatile-bitfields-2.c: New.
>
When I tried to make ARM EABI target default to 
-fstrict-volatile-bitfields, I found some regressions on ARM EABI caused 
by this flag:

http://gcc.gnu.org/ml/gcc-patches/2010-09/msg00155.html

h8300, sh and rx should also have the same regressions. m32c does not 
suffer it because it's not STRICT_ALIGNMENT.

There are already some discussion under that thread. And with some more 
offline discussion with Joseph, Dan and Paul, we think this flag should 
be fixed to fix the regressions, i.e. when packed attribute conflicts 
with -fstrict-volatile-bitfields, GCC should honor packed attribute. One 
reason is that -fstrict-volatile-bitfields should not cause any 
regression on testsuite. Another reason is that this is what armcc does 
and we'd better keep compatibility with armcc on this case.

BTW, it's a little confusing that -fstrict-volatile-bitfields causes a 
warning on code which does not use any bitfields.


Regards,
-- 
Jie Zhang
CodeSourcery