diff mbox

Prefer packed attribute when conflicts with volatile

Message ID 4CB866EA.3000709@codesourcery.com
State New
Headers show

Commit Message

Jie Zhang Oct. 15, 2010, 2:36 p.m. UTC
There is regression in GCC testsuite if -fstrict-volatile-bitfields is 
set default for ARM EABI.[1]  This patch means to fix that regression.

As discussion in the thread starting from [1] and as stated in [2], GCC 
should prefer packed attribute when it conflicts with 
-fstrict-volatile-bitfields. This patch is written according to this. It 
adds a new argument "packedp" to extract_bit_field and 
extract_fixed_bit_field. So we can pass packed information from TREE to 
RTL when expanding. When packed attribute conflicts with 
-fstrict-volatile-bitfields, we just prefer packed attribute.

Bootstrapped and regression tested with c,lto,c++ on 
x86_64-unknown-linux-gnu. Regression tested with c,lto,c++ on arm-none-eabi.

Is it OK?


[1] http://gcc.gnu.org/ml/gcc-patches/2010-09/msg00155.html
[2] http://gcc.gnu.org/ml/gcc-patches/2010-09/msg00287.html

Comments

Jie Zhang Oct. 19, 2010, 3:33 p.m. UTC | #1
Hi Eric & Richard,

I know it's too soon to send a ping. But there is another patch

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

waiting for this one. Could one of you give a review for this patch and 
also for the target independent part of the above one. Thanks!
Eric Botcazou Oct. 20, 2010, 5:35 p.m. UTC | #2
> I know it's too soon to send a ping. But there is another patch
>
> http://gcc.gnu.org/ml/gcc-patches/2010-09/msg00155.html
>
> waiting for this one. Could one of you give a review for this patch and
> also for the target independent part of the above one. Thanks!

Sorry, I cannot, this is outside the part of the compiler I maintain.
Jie Zhang Oct. 21, 2010, 1:58 a.m. UTC | #3
On 10/21/2010 01:35 AM, Eric Botcazou wrote:
>> I know it's too soon to send a ping. But there is another patch
>>
>> http://gcc.gnu.org/ml/gcc-patches/2010-09/msg00155.html
>>
>> waiting for this one. Could one of you give a review for this patch and
>> also for the target independent part of the above one. Thanks!
>
> Sorry, I cannot, this is outside the part of the compiler I maintain.
>
No problem. I can try someone else. The patch changes code in tree -> 
RTL. You are listed as the maintainer for RTL optimizers. I know it's 
not perfectly match but you and Richard Henderson are recommended for 
reviewing my patch on IRC. Maybe we should have a maintainer for expand 
pass?
Mark Mitchell Oct. 21, 2010, 4:57 a.m. UTC | #4
On 10/20/2010 6:58 PM, Jie Zhang wrote:

> No problem. I can try someone else. The patch changes code in tree ->
> RTL. You are listed as the maintainer for RTL optimizers. I know it's
> not perfectly match but you and Richard Henderson are recommended for
> reviewing my patch on IRC. Maybe we should have a maintainer for expand
> pass?

I think the patch makes sense, but I will pick a few nits. :-)

Algorithmically, why do we have to call extract_fixed_bit_field twice
(once with final clear and once with it set)?  Can't we issue the
warning and then just keep going?  Or, in the worst case, do a goto to
the top of the function?  Callers shouldn't have to handle this
final/not-final distinction.

Also, as a point of style, new boolean parameters should be declared as
"bool", not as "int".  So, for example, "extract_fixed_bit_field" should
have "packedp" as "bool".

(Also, although it's not specified as a standard GNU style, I like to write:

  ..., /*packedp=*/true, ...

when passing cryptic boolean arguments to functions; otherwise, it's
confusing to look at:

  f (true, false, 1, 0, false)

and work out what it's doing.  You can decide whether you want to use
that style or not.)

Please resubmit with those changes.

Thank you,
Eric Botcazou Oct. 21, 2010, 1:40 p.m. UTC | #5
> No problem. I can try someone else. The patch changes code in tree ->
> RTL. You are listed as the maintainer for RTL optimizers.

Old debate. :-)  See http://gcc.gnu.org/ml/gcc/2006-08/msg00106.html

> I know it's not perfectly match but you and Richard Henderson are
> recommended for reviewing my patch on IRC.

That Richard, or one of the others (Richard G.), can indeed do the review.

> Maybe we should have a maintainer for expand pass?

This historically has been subsumed into the middle-end maintainership.
Jie Zhang Oct. 21, 2010, 1:46 p.m. UTC | #6
On 10/21/2010 09:40 PM, Eric Botcazou wrote:
>> No problem. I can try someone else. The patch changes code in tree ->
>> RTL. You are listed as the maintainer for RTL optimizers.
>
> Old debate. :-)  See http://gcc.gnu.org/ml/gcc/2006-08/msg00106.html
>
Interesting. :-)

>> I know it's not perfectly match but you and Richard Henderson are
>> recommended for reviewing my patch on IRC.
>
> That Richard, or one of the others (Richard G.), can indeed do the review.
>
More interesting. It's Richard G. that recommended you and Richard H. ;-)

>> Maybe we should have a maintainer for expand pass?
>
> This historically has been subsumed into the middle-end maintainership.
>
OK.


Regards,
diff mbox

Patch

	
	* expr.c (emit_group_load_1): Update calls to extract_bit_field.
	(copy_blkmode_from_reg): Likewise.
	(read_complex_part): Likewise.
	(expand_expr_real_1): Calculate packedp and pass it to
	extract_bit_field.
	* expr.h (extract_bit_field): Update declaration.
	* calls.c (store_unaligned_arguments_into_pseudos): Update call
	to extract_bit_field.
	* expmed.c (extract_fixed_bit_field): Update calls to
	extract_fixed_bit_field.
	(store_split_bit_field): Likewise.
	(extract_bit_field_1): Add new argument packedp.
	(extract_bit_field): Add new argument packedp.
	(extract_fixed_bit_field_1): Split from ...
	(extract_fixed_bit_field): ... this. And add new argument packedp.
	* stmt.c (expand_return): Update call to extract_bit_field.

Index: expr.c
===================================================================
--- expr.c	(revision 165508)
+++ expr.c	(working copy)
@@ -1703,7 +1703,7 @@  emit_group_load_1 (rtx *tmps, rtx dst, r
 		  && (!REG_P (tmps[i]) || GET_MODE (tmps[i]) != mode))
 		tmps[i] = extract_bit_field (tmps[i], bytelen * BITS_PER_UNIT,
 					     (bytepos % slen0) * BITS_PER_UNIT,
-					     1, NULL_RTX, mode, mode);
+					     1, 0, NULL_RTX, mode, mode);
 	    }
 	  else
 	    {
@@ -1713,7 +1713,7 @@  emit_group_load_1 (rtx *tmps, rtx dst, r
 	      mem = assign_stack_temp (GET_MODE (src), slen, 0);
 	      emit_move_insn (mem, src);
 	      tmps[i] = extract_bit_field (mem, bytelen * BITS_PER_UNIT,
-					   0, 1, NULL_RTX, mode, mode);
+					   0, 1, 0, NULL_RTX, mode, mode);
 	    }
 	}
       /* FIXME: A SIMD parallel will eventually lead to a subreg of a
@@ -1754,7 +1754,7 @@  emit_group_load_1 (rtx *tmps, rtx dst, r
 	tmps[i] = src;
       else
 	tmps[i] = extract_bit_field (src, bytelen * BITS_PER_UNIT,
-				     bytepos * BITS_PER_UNIT, 1, NULL_RTX,
+				     bytepos * BITS_PER_UNIT, 1, 0, NULL_RTX,
 				     mode, mode);
 
       if (shift)
@@ -2167,7 +2167,7 @@  copy_blkmode_from_reg (rtx tgtblk, rtx s
 	 bitpos for the destination store (left justified).  */
       store_bit_field (dst, bitsize, bitpos % BITS_PER_WORD, copy_mode,
 		       extract_bit_field (src, bitsize,
-					  xbitpos % BITS_PER_WORD, 1,
+					  xbitpos % BITS_PER_WORD, 1, 0,
 					  NULL_RTX, copy_mode, copy_mode));
     }
 
@@ -2924,7 +2924,7 @@  read_complex_part (rtx cplx, bool imag_p
     }
 
   return extract_bit_field (cplx, ibitsize, imag_p ? ibitsize : 0,
-			    true, NULL_RTX, imode, imode);
+			    true, 0, NULL_RTX, imode, imode);
 }
 
 /* A subroutine of emit_move_insn_1.  Yet another lowpart generator.
@@ -8937,7 +8937,7 @@  expand_expr_real_1 (tree exp, rtx target
 	enum machine_mode mode1, mode2;
 	HOST_WIDE_INT bitsize, bitpos;
 	tree offset;
-	int volatilep = 0, must_force_mem;
+	int volatilep = 0, packedp = 0, must_force_mem;
 	tree tem = get_inner_reference (exp, &bitsize, &bitpos, &offset,
 					&mode1, &unsignedp, &volatilep, true);
 	rtx orig_op0, memloc;
@@ -8947,6 +8947,11 @@  expand_expr_real_1 (tree exp, rtx target
 	   infinitely recurse.  */
 	gcc_assert (tem != exp);
 
+	if (TYPE_PACKED (TREE_TYPE (TREE_OPERAND (exp, 0)))
+	    || (TREE_CODE (TREE_OPERAND (exp, 1)) == FIELD_DECL
+		&& DECL_PACKED (TREE_OPERAND (exp, 1))))
+	  packedp = 1;
+
 	/* If TEM's type is a union of variable size, pass TARGET to the inner
 	   computation, since it will need a temporary and TARGET is known
 	   to have to do.  This occurs in unchecked conversion in Ada.  */
@@ -9159,7 +9164,7 @@  expand_expr_real_1 (tree exp, rtx target
 	    if (MEM_P (op0) && REG_P (XEXP (op0, 0)))
 	      mark_reg_pointer (XEXP (op0, 0), MEM_ALIGN (op0));
 
-	    op0 = extract_bit_field (op0, bitsize, bitpos, unsignedp,
+	    op0 = extract_bit_field (op0, bitsize, bitpos, unsignedp, packedp,
 				     (modifier == EXPAND_STACK_PARM
 				      ? NULL_RTX : target),
 				     ext_mode, ext_mode);
Index: expr.h
===================================================================
--- expr.h	(revision 165508)
+++ expr.h	(working copy)
@@ -668,7 +668,7 @@  mode_for_extraction (enum extraction_pat
 extern void store_bit_field (rtx, unsigned HOST_WIDE_INT,
 			     unsigned HOST_WIDE_INT, enum machine_mode, rtx);
 extern rtx extract_bit_field (rtx, unsigned HOST_WIDE_INT,
-			      unsigned HOST_WIDE_INT, int, rtx,
+			      unsigned HOST_WIDE_INT, int, int, rtx,
 			      enum machine_mode, enum machine_mode);
 extern rtx extract_low_bits (enum machine_mode, enum machine_mode, rtx);
 extern rtx expand_mult (enum machine_mode, rtx, rtx, rtx, int);
Index: calls.c
===================================================================
--- calls.c	(revision 165508)
+++ calls.c	(working copy)
@@ -886,7 +886,7 @@  store_unaligned_arguments_into_pseudos (
 	    int bitsize = MIN (bytes * BITS_PER_UNIT, BITS_PER_WORD);
 
 	    args[i].aligned_regs[j] = reg;
-	    word = extract_bit_field (word, bitsize, 0, 1, NULL_RTX,
+	    word = extract_bit_field (word, bitsize, 0, 1, 0, NULL_RTX,
 				      word_mode, word_mode);
 
 	    /* There is no need to restrict this code to loading items
Index: expmed.c
===================================================================
--- expmed.c	(revision 165508)
+++ expmed.c	(working copy)
@@ -53,7 +53,7 @@  static void store_split_bit_field (rtx,
 static rtx extract_fixed_bit_field (enum machine_mode, rtx,
 				    unsigned HOST_WIDE_INT,
 				    unsigned HOST_WIDE_INT,
-				    unsigned HOST_WIDE_INT, rtx, int);
+				    unsigned HOST_WIDE_INT, rtx, int, int);
 static rtx mask_rtx (enum machine_mode, int, int, int);
 static rtx lshift_value (enum machine_mode, rtx, int, int);
 static rtx extract_split_bit_field (rtx, unsigned HOST_WIDE_INT,
@@ -1083,7 +1083,7 @@  store_split_bit_field (rtx op0, unsigned
 	       endianness compensation) to fetch the piece we want.  */
 	    part = extract_fixed_bit_field (word_mode, value, 0, thissize,
 					    total_bits - bitsize + bitsdone,
-					    NULL_RTX, 1);
+					    NULL_RTX, 1, 0);
 	}
       else
 	{
@@ -1094,7 +1094,7 @@  store_split_bit_field (rtx op0, unsigned
 			    & (((HOST_WIDE_INT) 1 << thissize) - 1));
 	  else
 	    part = extract_fixed_bit_field (word_mode, value, 0, thissize,
-					    bitsdone, NULL_RTX, 1);
+					    bitsdone, NULL_RTX, 1, 0);
 	}
 
       /* If OP0 is a register, then handle OFFSET here.
@@ -1160,7 +1160,8 @@  convert_extracted_bit_field (rtx x, enum
 
 static rtx
 extract_bit_field_1 (rtx str_rtx, unsigned HOST_WIDE_INT bitsize,
-		     unsigned HOST_WIDE_INT bitnum, int unsignedp, rtx target,
+		     unsigned HOST_WIDE_INT bitnum,
+		     int unsignedp, int packedp, rtx target,
 		     enum machine_mode mode, enum machine_mode tmode,
 		     bool fallback_p)
 {
@@ -1441,7 +1442,7 @@  extract_bit_field_1 (rtx str_rtx, unsign
 	  rtx result_part
 	    = extract_bit_field (op0, MIN (BITS_PER_WORD,
 					   bitsize - i * BITS_PER_WORD),
-				 bitnum + bit_offset, 1, target_part, mode,
+				 bitnum + bit_offset, 1, 0, target_part, mode,
 				 word_mode);
 
 	  gcc_assert (target_part);
@@ -1640,7 +1641,7 @@  extract_bit_field_1 (rtx str_rtx, unsign
 	      xop0 = adjust_address (op0, bestmode, xoffset);
 	      xop0 = force_reg (bestmode, xop0);
 	      result = extract_bit_field_1 (xop0, bitsize, xbitpos,
-					    unsignedp, target,
+					    unsignedp, packedp, target,
 					    mode, tmode, false);
 	      if (result)
 		return result;
@@ -1654,7 +1655,7 @@  extract_bit_field_1 (rtx str_rtx, unsign
     return NULL;
 
   target = extract_fixed_bit_field (int_mode, op0, offset, bitsize,
-				    bitpos, target, unsignedp);
+				    bitpos, target, unsignedp, packedp);
   return convert_extracted_bit_field (target, mode, tmode, unsignedp);
 }
 
@@ -1665,6 +1666,7 @@  extract_bit_field_1 (rtx str_rtx, unsign
 
    STR_RTX is the structure containing the byte (a REG or MEM).
    UNSIGNEDP is nonzero if this is an unsigned bit field.
+   PACKEDP is nonzero if the field has the packed attribute.
    MODE is the natural mode of the field value once extracted.
    TMODE is the mode the caller would like the value to have;
    but the value may be returned with type MODE instead.
@@ -1676,35 +1678,28 @@  extract_bit_field_1 (rtx str_rtx, unsign
 
 rtx
 extract_bit_field (rtx str_rtx, unsigned HOST_WIDE_INT bitsize,
-		   unsigned HOST_WIDE_INT bitnum, int unsignedp, rtx target,
-		   enum machine_mode mode, enum machine_mode tmode)
+		   unsigned HOST_WIDE_INT bitnum, int unsignedp, int packedp,
+		   rtx target, enum machine_mode mode, enum machine_mode tmode)
 {
-  return extract_bit_field_1 (str_rtx, bitsize, bitnum, unsignedp,
+  return extract_bit_field_1 (str_rtx, bitsize, bitnum, unsignedp, packedp,
 			      target, mode, tmode, true);
 }
 
-/* Extract a bit field using shifts and boolean operations
-   Returns an rtx to represent the value.
-   OP0 addresses a register (word) or memory (byte).
-   BITPOS says which bit within the word or byte the bit field starts in.
-   OFFSET says how many bytes farther the bit field starts;
-    it is 0 if OP0 is a register.
-   BITSIZE says how many bits long the bit field is.
-    (If OP0 is a register, it may be narrower than a full word,
-     but BITPOS still counts within a full word,
-     which is significant on bigendian machines.)
+/* A subroutine of extract_fixed_bit_field, with the same arguments.
 
-   UNSIGNEDP is nonzero for an unsigned bit field (don't sign-extend value).
-   If TARGET is nonzero, attempts to store the value there
-   and return TARGET, but this is not guaranteed.
-   If TARGET is not used, create a pseudo-reg of mode TMODE for the value.  */
+   The function will be called twice for a packed bit-field.
+   When it's called for the first time, FINAL is set to 0 and
+   it tries to preserve the volatility if -fstrict-volatile-bitfields.
+   If the volatility can't be preserved and PACKEDP is nonzero, it will
+   be called for the second time with FINAL set to nonzero to override
+   -fstrict-volatile-bitfields with packed attribute.  */
 
 static rtx
-extract_fixed_bit_field (enum machine_mode tmode, rtx op0,
-			 unsigned HOST_WIDE_INT offset,
-			 unsigned HOST_WIDE_INT bitsize,
-			 unsigned HOST_WIDE_INT bitpos, rtx target,
-			 int unsignedp)
+extract_fixed_bit_field_1 (enum machine_mode tmode, rtx op0,
+			   unsigned HOST_WIDE_INT offset,
+			   unsigned HOST_WIDE_INT bitsize,
+			   unsigned HOST_WIDE_INT bitpos, rtx target,
+			   int unsignedp, int packedp, int final)
 {
   unsigned int total_bits = BITS_PER_WORD;
   enum machine_mode mode;
@@ -1722,7 +1717,8 @@  extract_fixed_bit_field (enum machine_mo
 	 a word, we won't be doing the extraction the normal way.  */
 
       if (MEM_VOLATILE_P (op0)
-	  && flag_strict_volatile_bitfields > 0)
+	  && flag_strict_volatile_bitfields > 0
+	  && !(packedp && final))
 	{
 	  if (GET_MODE_BITSIZE (GET_MODE (op0)) > 0)
 	    mode = GET_MODE (op0);
@@ -1761,6 +1757,7 @@  extract_fixed_bit_field (enum machine_mo
       if (MEM_P (op0)
 	  && MEM_VOLATILE_P (op0)
 	  && flag_strict_volatile_bitfields > 0
+	  && !(packedp && final)
 	  && bitpos + bitsize <= total_bits
 	  && bitpos + bitsize + (offset % (total_bits / BITS_PER_UNIT)) * BITS_PER_UNIT > total_bits)
 	{
@@ -1769,6 +1766,20 @@  extract_fixed_bit_field (enum machine_mo
 	      static bool informed_about_misalignment = false;
 	      bool warned;
 
+	      if (packedp && !final)
+		{
+		  if (bitsize == total_bits)
+		    warned = warning_at (input_location, OPT_fstrict_volatile_bitfields,
+					 "multiple accesses to volatile structure member"
+					 " because of packed attribute");
+		  else
+		    warned = warning_at (input_location, OPT_fstrict_volatile_bitfields,
+					 "multiple accesses to volatile structure bitfield"
+					 " because of packed attribute");
+
+		  return NULL_RTX;
+		}
+
 	      if (bitsize == total_bits)
 		warned = warning_at (input_location, OPT_fstrict_volatile_bitfields,
 				     "mis-aligned access used for structure member");
@@ -1870,6 +1881,44 @@  extract_fixed_bit_field (enum machine_mo
 				      GET_MODE_BITSIZE (mode) - bitsize),
 		       target, 0);
 }
+
+/* Extract a bit field using shifts and boolean operations
+   Returns an rtx to represent the value.
+   OP0 addresses a register (word) or memory (byte).
+   BITPOS says which bit within the word or byte the bit field starts in.
+   OFFSET says how many bytes farther the bit field starts;
+    it is 0 if OP0 is a register.
+   BITSIZE says how many bits long the bit field is.
+    (If OP0 is a register, it may be narrower than a full word,
+     but BITPOS still counts within a full word,
+     which is significant on bigendian machines.)
+
+   UNSIGNEDP is nonzero for an unsigned bit field (don't sign-extend value).
+   PACKEDP is nonezero if the field has the packed attribute.
+
+   If TARGET is nonzero, attempts to store the value there
+   and return TARGET, but this is not guaranteed.
+   If TARGET is not used, create a pseudo-reg of mode TMODE for the value.  */
+
+static rtx
+extract_fixed_bit_field (enum machine_mode tmode, rtx op0,
+			 unsigned HOST_WIDE_INT offset,
+			 unsigned HOST_WIDE_INT bitsize,
+			 unsigned HOST_WIDE_INT bitpos, rtx target,
+			 int unsignedp, int packedp)
+{
+  if (packedp)
+    {
+      rtx result = extract_fixed_bit_field_1 (tmode, op0, offset, bitsize,
+					      bitpos, target, unsignedp, 1, 0);
+      if (result)
+	return result;
+    }
+
+  return extract_fixed_bit_field_1 (tmode, op0, offset, bitsize,
+				    bitpos, target, unsignedp, packedp, 1);
+}
+
 
 /* Return a constant integer (CONST_INT or CONST_DOUBLE) mask value
    of mode MODE with BITSIZE ones followed by BITPOS zeros, or the
@@ -1971,7 +2020,7 @@  extract_split_bit_field (rtx op0, unsign
 	 extract_fixed_bit_field wants offset in bytes.  */
       part = extract_fixed_bit_field (word_mode, word,
 				      offset * unit / BITS_PER_UNIT,
-				      thissize, thispos, 0, 1);
+				      thissize, thispos, 0, 1, 0);
       bitsdone += thissize;
 
       /* Shift this part into place for the result.  */
Index: stmt.c
===================================================================
--- stmt.c	(revision 165508)
+++ stmt.c	(working copy)
@@ -1739,7 +1739,7 @@  expand_return (tree retval)
 	     xbitpos for the destination store (right justified).  */
 	  store_bit_field (dst, bitsize, xbitpos % BITS_PER_WORD, word_mode,
 			   extract_bit_field (src, bitsize,
-					      bitpos % BITS_PER_WORD, 1,
+					      bitpos % BITS_PER_WORD, 1, 0,
 					      NULL_RTX, word_mode, word_mode));
 	}