Re: [patch] Width of volatile bitfields (PR23623)

[Paul Brook <paul at codesourcery dot com>]

On Tuesday 28 March 2006 16:31, Paul Brook wrote:
> The patch below attempts to fix PR23623.
>
> The problem is that there are two conflicting definitions of the semantics
> of volatile bitfields:
>
> a) The container type should determine the type of the memory access. This
> allows the user explicit control over the access, which is typically useful
> for memory mapped peripherals.
> b) The narrowest single access should be used. This is the historic
> behavior. I guess this is to avoid superfluous memory accesses for
> unaligned or wider than memory bus accesses.
>
> Given the definitions above I guess that STRICT_ALIGNMENT is as good a way
> as any to differentiate between the two behaviors. I'm open to other
> suggestions on how to control this behavior (eg. add a target hook). My
> only real criteria is that the ARM EABI requires behavior (a).

Richard Earnshaw pointed out offline that there's no guarantee that Arm will 
always be STRICT_ALIGN. Patch below adds a target hook and overrides that for 
Arm. Otherwise the semantics are the same as described above.

Tested with cross to arm-none-eabi, and spot check on i686-inux.
Ok?

Paul

2006-03-28  Paul Brook  <paul@codesourcery.com>

	* targhooks.c (default_narrow_bitfield): New fuction.
	* targhooks.h (default_narrow_bitfield): add prototype.
	* target-def.h (TARGET_NARROW_VOLATILE_BITFIELD): Define.
	* doc/tm.texi: Document TARGET_NARROW_VOLATILE_BITFIELDS.
	* config/arm/arm.c (TARGET_NARROW_VOLATILE_BITFIELD): Define.

Index: gcc/doc/tm.texi
===================================================================
--- gcc/doc/tm.texi	(revision 112423)
+++ gcc/doc/tm.texi	(working copy)
@@ -1212,6 +1212,14 @@ structure.  The hook should return true 
 the alignment requirements of an unnamed bitfield's type.
 @end deftypefn
 
+@deftypefn {Target Hook} bool TARGET_NARROW_VOLATILE_BITFIELDS (void)
+This target hook should return @code{true} if assesses to volatile bitfields
+should use the narrowest mode possible.  It should return @code{false} if
+these accesses should use the bitfield container type.
+
+The default is @code{!TARGET_STRICT_ALIGN}.
+@end deftypefn
+
 @defmac MEMBER_TYPE_FORCES_BLK (@var{field}, @var{mode})
 Return 1 if a structure or array containing @var{field} should be accessed 
using
 @code{BLKMODE}.
Index: gcc/targhooks.c
===================================================================
--- gcc/targhooks.c	(revision 112423)
+++ gcc/targhooks.c	(working copy)
@@ -463,4 +463,18 @@ default_internal_arg_pointer (void)
     return virtual_incoming_args_rtx;
 }
 
+
+/* If STRICT_ALIGNMENT is true we use the container type for accessing
+   volatile bitfields.  This is generally the prefered behavior for memory
+   mapped peripherals on RISC architectures.
+   If STRICT_ALIGNMENT is false we use the narrowest type possible.  This
+   is typically used to avoid spurious page faults and extra memory accesses
+   due to unaligned accesses on CISC architectures.  */
+
+bool
+default_narrow_bitfield (void)
+{
+  return !STRICT_ALIGNMENT;
+}
+
 #include "gt-targhooks.h"
Index: gcc/targhooks.h
===================================================================
--- gcc/targhooks.h	(revision 112423)
+++ gcc/targhooks.h	(working copy)
@@ -53,6 +53,8 @@ extern bool default_scalar_mode_supporte
 
 extern const char * default_invalid_within_doloop (rtx);
 
+extern bool default_narrow_bitfield (void);
+
 /* These are here, and not in hooks.[ch], because not all users of
    hooks.h include tm.h, and thus we don't have CUMULATIVE_ARGS.  */
 
Index: gcc/target.h
===================================================================
--- gcc/target.h	(revision 112423)
+++ gcc/target.h	(working copy)
@@ -344,6 +344,10 @@ struct gcc_target
   /* Return true if anonymous bitfields affect structure alignment.  */
   bool (* align_anon_bitfield) (void);
 
+  /* Return true if volatile bitfields should use the narrowest type 
possible.
+     Return false if they should use the container type.  */
+  bool (* narrow_volatile_bitfield) (void);
+
   /* Set up target-specific built-in functions.  */
   void (* init_builtins) (void);
 
Index: gcc/stor-layout.c
===================================================================
--- gcc/stor-layout.c	(revision 112423)
+++ gcc/stor-layout.c	(working copy)
@@ -2126,13 +2126,17 @@ fixup_unsigned_type (tree type)
    If LARGEST_MODE is not VOIDmode, it means that we should not use a mode
    larger than LARGEST_MODE (usually SImode).
 
-   If no mode meets all these conditions, we return VOIDmode.  Otherwise, if
-   VOLATILEP is true or SLOW_BYTE_ACCESS is false, we return the smallest
-   mode meeting these conditions.
-
-   Otherwise (VOLATILEP is false and SLOW_BYTE_ACCESS is true), we return
-   the largest mode (but a mode no wider than UNITS_PER_WORD) that meets
-   all the conditions.  */
+   If no mode meets all these conditions, we return VOIDmode.
+   
+   If VOLATILEP is false and SLOW_BYTE_ACCESS is false, we return the
+   smallest mode meeting these conditions.
+
+   If VOLATILEP is false and SLOW_BYTE_ACCESS is true, we return the
+   largest mode (but a mode no wider than UNITS_PER_WORD) that meets
+   all the conditions.
+   
+   If VOLATILEP is true the narrow_volatile_bitfields target hook is used to
+   decide which of the above modes should be used.  */
 
 enum machine_mode
 get_best_mode (int bitsize, int bitpos, unsigned int align,
@@ -2162,7 +2166,8 @@ get_best_mode (int bitsize, int bitpos, 
       || (largest_mode != VOIDmode && unit > GET_MODE_BITSIZE 
(largest_mode)))
     return VOIDmode;
 
-  if (SLOW_BYTE_ACCESS && ! volatilep)
+  if ((SLOW_BYTE_ACCESS && ! volatilep)
+      || (volatilep && !targetm.narrow_volatile_bitfield()))
     {
       enum machine_mode wide_mode = VOIDmode, tmode;
 
Index: gcc/target-def.h
===================================================================
--- gcc/target-def.h	(revision 112423)
+++ gcc/target-def.h	(working copy)
@@ -371,6 +371,7 @@ Foundation, 51 Franklin Street, Fifth Fl
 #define TARGET_FUNCTION_ATTRIBUTE_INLINABLE_P hook_bool_tree_false
 #define TARGET_MS_BITFIELD_LAYOUT_P hook_bool_tree_false
 #define TARGET_ALIGN_ANON_BITFIELD hook_bool_void_false
+#define TARGET_NARROW_VOLATILE_BITFIELD hook_bool_void_false
 #define TARGET_RTX_COSTS hook_bool_rtx_int_int_intp_false
 #define TARGET_MANGLE_FUNDAMENTAL_TYPE hook_constcharptr_tree_null
 #define TARGET_ALLOCATE_INITIAL_VALUE NULL
@@ -563,6 +564,7 @@ Foundation, 51 Franklin Street, Fifth Fl
   TARGET_FUNCTION_ATTRIBUTE_INLINABLE_P,	\
   TARGET_MS_BITFIELD_LAYOUT_P,			\
   TARGET_ALIGN_ANON_BITFIELD,			\
+  TARGET_NARROW_VOLATILE_BITFIELD,		\
   TARGET_INIT_BUILTINS,				\
   TARGET_EXPAND_BUILTIN,			\
   TARGET_RESOLVE_OVERLOADED_BUILTIN,		\
Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	(revision 112423)
+++ gcc/config/arm/arm.c	(working copy)
@@ -316,6 +316,9 @@ static bool arm_tls_symbol_p (rtx x);
 #undef TARGET_ALIGN_ANON_BITFIELD
 #define TARGET_ALIGN_ANON_BITFIELD arm_align_anon_bitfield
 
+#undef TARGET_NARROW_VOLATILE_BITFIELD
+#define TARGET_NARROW_VOLATILE_BITFIELD hook_bool_void_false
+
 #undef TARGET_CXX_GUARD_TYPE
 #define TARGET_CXX_GUARD_TYPE arm_cxx_guard_type
 
Index: gcc/config/arm/vfp.md
===================================================================
--- gcc/config/arm/vfp.md	(revision 112423)
+++ gcc/config/arm/vfp.md	(working copy)
@@ -232,10 +232,12 @@ (define_insn "*thumb2_movdi_vfp"
 
 
 ;; SFmode moves
+;; Disparage the w<->r cases because reloading an invalid address is
+;; preferable to loading the value via integer registers.
 
 (define_insn "*movsf_vfp"
-  [(set (match_operand:SF 0 "nonimmediate_operand" "=w,r,w  ,Uv,r ,m,w,r")
-	(match_operand:SF 1 "general_operand"	   " r,w,UvE,w, mE,r,w,r"))]
+  [(set (match_operand:SF 0 "nonimmediate_operand" "=w,?r,w  ,Uv,r ,m,w,r")
+	(match_operand:SF 1 "general_operand"	   " ?r,w,UvE,w, mE,r,w,r"))]
   "TARGET_ARM && TARGET_HARD_FLOAT && TARGET_VFP
    && (   s_register_operand (operands[0], SFmode)
        || s_register_operand (operands[1], SFmode))"
@@ -255,8 +257,8 @@ (define_insn "*movsf_vfp"
 )
 
 (define_insn "*thumb2_movsf_vfp"
-  [(set (match_operand:SF 0 "nonimmediate_operand" "=w,r,w  ,Uv,r ,m,w,r")
-	(match_operand:SF 1 "general_operand"	   " r,w,UvE,w, mE,r,w,r"))]
+  [(set (match_operand:SF 0 "nonimmediate_operand" "=w,?r,w  ,Uv,r ,m,w,r")
+	(match_operand:SF 1 "general_operand"	   " ?r,w,UvE,w, mE,r,w,r"))]
   "TARGET_THUMB2 && TARGET_HARD_FLOAT && TARGET_VFP
    && (   s_register_operand (operands[0], SFmode)
        || s_register_operand (operands[1], SFmode))"
@@ -279,8 +281,8 @@ (define_insn "*thumb2_movsf_vfp"
 ;; DFmode moves
 
 (define_insn "*movdf_vfp"
-  [(set (match_operand:DF 0 "nonimmediate_soft_df_operand" "=w,r,r, 
m,w  ,Uv,w,r")
-	(match_operand:DF 1 "soft_df_operand"		   " r,w,mF,r,UvF,w, w,r"))]
+  [(set (match_operand:DF 0 "nonimmediate_soft_df_operand" "=w,?r,r, 
m,w  ,Uv,w,r")
+	(match_operand:DF 1 "soft_df_operand"		   " ?r,w,mF,r,UvF,w, w,r"))]
   "TARGET_ARM && TARGET_HARD_FLOAT && TARGET_VFP
    && (   register_operand (operands[0], DFmode)
        || register_operand (operands[1], DFmode))"
@@ -314,8 +316,8 @@ (define_insn "*movdf_vfp"
 )
 
 (define_insn "*thumb2_movdf_vfp"
-  [(set (match_operand:DF 0 "nonimmediate_soft_df_operand" "=w,r,r, 
m,w  ,Uv,w,r")
-	(match_operand:DF 1 "soft_df_operand"		   " r,w,mF,r,UvF,w, w,r"))]
+  [(set (match_operand:DF 0 "nonimmediate_soft_df_operand" "=w,?r,r, 
m,w  ,Uv,w,r")
+	(match_operand:DF 1 "soft_df_operand"		   " ?r,w,mF,r,UvF,w, w,r"))]
   "TARGET_THUMB2 && TARGET_HARD_FLOAT && TARGET_VFP"
   "*
   {