diff mbox

PING: Default to -fstrict-volatile-bitfields for ARM EABI

Message ID 4CC3A42E.3000405@codesourcery.com
State New
Headers show

Commit Message

Jie Zhang Oct. 24, 2010, 3:12 a.m. UTC 
On 10/23/2010 02:03 AM, Jie Zhang wrote:
> On 10/22/2010 05:40 PM, Richard Guenther wrote:
>> On Fri, 22 Oct 2010, Jie Zhang wrote:
>>
>>> Since the regression has been fixed now, it's time to reconsider this
>>> patch:
>>>
>>> http://gcc.gnu.org/ml/gcc-patches/2010-09/msg00155.html
>>>
>>> Regression tested on latest trunk for arm-none-eabi with c,lto,c++.
>>>
>>> ARM maintainers: is the ARM port part OK?
>>>
>>> Richard G: Could you help me review the stor-layout.c part?
>>
>> Why is the stor-layout.c part necessary? Why does it not possibly
>> cause layout changes? I'm not very familiar with the stor-layout.c
>> code, and given that questions I think I'm not the right person
>> to review this patch.
>
> Thanks for your questions! It's not necessary indeed. I just retested
> the patch with the stor-layout.c change dropped. It has no regressions
> on arm-none-eabi with c,c++,lto and it passes all three new tests. I
> don't remember the exact details why I changed stor-layout.c at that
> time. So now the patch is only for ARM port. The updated patch is attached.
>
I have been trying to recall the reason of the stor-layout.c change for 
the last two days. Now I know. The regular expressions used in the three 
new new tests have a bug. The "." matches "\n" in expect, so "ldr\[\\t 
\]+.*,\[\\t \]*\\\[.*\\\]" will match

         ldr     r3, .L2
         ldrb    r0, [r3, #1]    @ zero_extendqisi2

but it's intended to match one instruction like

         ldr    r0, [r3, #0]

So the PASSes are fake after dropping the stor-layout.c change.

Now I can explain the stor-layout.c change. When layout_decl processes 
"b" bit-field in

typedef struct {
   volatile unsigned long a:8;
   volatile unsigned long b:8;
   volatile unsigned long c:16;
} BitStruct;

, this piece of code

	  /* See if we can use an ordinary integer mode for a bit-field.
	     Conditions are: a fixed size that is correct for another mode,
	     occupying a complete byte or bytes on proper boundary,
	     and not volatile or not -fstrict-volatile-bitfields.  */
	  if (TYPE_SIZE (type) != 0
	      && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST
	      && GET_MODE_CLASS (TYPE_MODE (type)) == MODE_INT)
	    {
	      enum machine_mode xmode
		= mode_for_size_tree (DECL_SIZE (decl), MODE_INT, 1);
	      unsigned int xalign = GET_MODE_ALIGNMENT (xmode);

	      if (xmode != BLKmode
		  && !(xalign > BITS_PER_UNIT && DECL_PACKED (decl))
		  && (known_align == 0 || known_align >= xalign))
		{
		  DECL_ALIGN (decl) = MAX (xalign, DECL_ALIGN (decl));
		  DECL_MODE (decl) = xmode;
		  DECL_BIT_FIELD (decl) = 0;
		}
	    }

will change the mode of "b" to QImode and clear DECL_BIT_FIELD (decl). 
Then later access to "b" will use byte load instruction instead of word 
load instruction as required by -fstrict-volatile-bitfields.

The attached patch fixes the tests and is resubmitted for approval for 
stor-layout.c change. Is it OK?


Regards,
diff mbox

Patch


	* stor-layout.c (layout_decl): Use the field's type to
	determine the mode and keep DECL_BIT_FIELD for a volatile
	bit-field.
	* config/arm/arm.c (arm_override_options): Default to
	-fstrict-volatile-bitfields.

	testsuite/
	* gcc.target/arm/volatile-bitfields-1.c: New test.
	* gcc.target/arm/volatile-bitfields-2.c: New test.
	* gcc.target/arm/volatile-bitfields-3.c: New test.

Index: testsuite/gcc.target/arm/volatile-bitfields-1.c
===================================================================
--- testsuite/gcc.target/arm/volatile-bitfields-1.c	(revision 0)
+++ testsuite/gcc.target/arm/volatile-bitfields-1.c	(revision 0)
@@ -0,0 +1,18 @@ 
+/* { dg-require-effective-target arm_eabi } */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+typedef struct {
+  char a:1;
+  char b:7;
+  int c;
+} BitStruct;
+
+volatile BitStruct bits;
+
+int foo ()
+{
+  return bits.b;
+}
+
+/* { dg-final { scan-assembler "ldrb\[\\t \]+\[^\n\]*,\[\\t \]*\\\[\[^\n\]*\\\]" } } */
Index: testsuite/gcc.target/arm/volatile-bitfields-2.c
===================================================================
--- testsuite/gcc.target/arm/volatile-bitfields-2.c	(revision 0)
+++ testsuite/gcc.target/arm/volatile-bitfields-2.c	(revision 0)
@@ -0,0 +1,18 @@ 
+/* { dg-require-effective-target arm_eabi } */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+typedef struct {
+  volatile unsigned long a:8;
+  volatile unsigned long b:8;
+  volatile unsigned long c:16;
+} BitStruct;
+
+BitStruct bits;
+
+unsigned long foo ()
+{
+  return bits.b;
+}
+
+/* { dg-final { scan-assembler "ldr\[\\t \]+\[^\n\]*,\[\\t \]*\\\[\[^\n\]*\\\]" } } */
Index: testsuite/gcc.target/arm/volatile-bitfields-3.c
===================================================================
--- testsuite/gcc.target/arm/volatile-bitfields-3.c	(revision 0)
+++ testsuite/gcc.target/arm/volatile-bitfields-3.c	(revision 0)
@@ -0,0 +1,18 @@ 
+/* { dg-require-effective-target arm_eabi } */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+typedef struct {
+  volatile unsigned long a:8;
+  volatile unsigned long b:8;
+  volatile unsigned long c:16;
+} BitStruct;
+
+BitStruct bits;
+
+unsigned long foo ()
+{
+  return bits.c;
+}
+
+/* { dg-final { scan-assembler "ldr\[\\t \]+\[^\n\]*,\[\\t \]*\\\[\[^\n\]*\\\]" } } */
Index: stor-layout.c
===================================================================
--- stor-layout.c	(revision 165879)
+++ stor-layout.c	(working copy)
@@ -661,11 +661,14 @@  layout_decl (tree decl, unsigned int kno
 	    }
 
 	  /* See if we can use an ordinary integer mode for a bit-field.
-	     Conditions are: a fixed size that is correct for another mode
-	     and occupying a complete byte or bytes on proper boundary.  */
+	     Conditions are: a fixed size that is correct for another mode,
+	     occupying a complete byte or bytes on proper boundary,
+	     and not volatile or not -fstrict-volatile-bitfields.  */
 	  if (TYPE_SIZE (type) != 0
 	      && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST
-	      && GET_MODE_CLASS (TYPE_MODE (type)) == MODE_INT)
+	      && GET_MODE_CLASS (TYPE_MODE (type)) == MODE_INT
+	      && !(TREE_THIS_VOLATILE (decl)
+		   && flag_strict_volatile_bitfields > 0))
 	    {
 	      enum machine_mode xmode
 		= mode_for_size_tree (DECL_SIZE (decl), MODE_INT, 1);
Index: config/arm/arm.c
===================================================================
--- config/arm/arm.c	(revision 165879)
+++ config/arm/arm.c	(working copy)
@@ -1971,6 +1971,10 @@  arm_option_override (void)
 			   global_options.x_param_values,
 			   global_options_set.x_param_values);
 
+  /* ARM EABI defaults to strict volatile bitfields.  */
+  if (TARGET_AAPCS_BASED && flag_strict_volatile_bitfields < 0)
+    flag_strict_volatile_bitfields = 1;
+
   /* Register global variables with the garbage collector.  */
   arm_add_gc_roots ();
 }