Patchwork PR50325 store_bit_field: Fix for big endian targets

login
register
mail settings
Submitter Richard Sandiford
Date Jan. 17, 2012, 8:38 p.m.
Message ID <8762ga6p5j.fsf@firetop.home>
Download mbox | patch
Permalink /patch/136525/
State New
Headers show

Comments

Richard Sandiford - Jan. 17, 2012, 8:38 p.m.
Richard Sandiford <rdsandiford@googlemail.com> writes:
> Iain Sandoe <developer@sandoe-acoustics.co.uk> writes:
>> On 17 Nov 2011, at 09:25, Andreas Krebbel wrote:
>>> On 11/17/2011 03:44 AM, David Edelsohn wrote:
>>>> Andreas,
>>>>
>>>> This patch seems to have introduced a failure for all of the
>>>> gcc.dg-struct-layout tests on AIX.
>>>>
>>>> gcc.dg-struct-layout-1/t001_test.h:8:1: internal compiler error: in
>>>> int_mode_for_mode, at stor-layout.c:424
>>>>
>>>> After your change, int_mode_for_mode now is passed VOIDmode because
>>>> the rtx is a CONST_INT.
>>>
>>> extract_bit_field is only able to deal with regs and mems. For  
>>> constants it should not be
>>> necessary anyway. Could you please test the following patch:
>>>
>>> Index: gcc/expmed.c
>>> ===================================================================
>>> *** gcc/expmed.c.orig   2011-11-15 20:03:46.000000000 +0100
>>> --- gcc/expmed.c        2011-11-17 09:12:22.487783491 +0100
>>> *************** store_bit_field_1 (rtx str_rtx, unsigned
>>> *** 562,570 ****
>>>            MIN (BITS_PER_WORD, bitsize - i * BITS_PER_WORD);
>>>
>>>          /* If the remaining chunk doesn't have full wordsize we have
>>> !            to make that for big endian machines the higher order
>>> !            bits are used.  */
>>> !         if (new_bitsize < BITS_PER_WORD && BYTES_BIG_ENDIAN)
>>>            value_word = extract_bit_field (value_word, new_bitsize, 0,
>>>                                            true, false, NULL_RTX,
>>>                                            BLKmode, word_mode);
>>> --- 562,572 ----
>>>            MIN (BITS_PER_WORD, bitsize - i * BITS_PER_WORD);
>>>
>>>          /* If the remaining chunk doesn't have full wordsize we have
>>> !            to make sure that for big endian machines the higher
>>> !            order bits are used.  */
>>> !         if (BYTES_BIG_ENDIAN
>>> !             && GET_MODE (value_word) != VOIDmode
>>> !             && new_bitsize < BITS_PER_WORD)
>>>            value_word = extract_bit_field (value_word, new_bitsize, 0,
>>>                                            true, false, NULL_RTX,
>>>                                            BLKmode, word_mode);
>>>
>>
>> with this + r181436 on powerpc-darwin9, the ICEs are gone ..
>> .. but all the struct-layout-1 tests are failing on execute.
>>
>> Running /GCC/gcc-live-trunk/gcc/testsuite/gcc.dg/compat/struct- 
>> layout-1.exp ...
>> FAIL: tmpdir-gcc.dg-struct-layout-1/t001 c_compat_x_tst.o- 
>> c_compat_y_tst.o execute
>> FAIL: tmpdir-gcc.dg-struct-layout-1/t002 c_compat_x_tst.o- 
>> c_compat_y_tst.o execute
>> FAIL: tmpdir-gcc.dg-struct-layout-1/t003 c_compat_x_tst.o- 
>> c_compat_y_tst.o execute
>> FAIL: tmpdir-gcc.dg-struct-layout-1/t004 c_compat_x_tst.o- 
>> c_compat_y_tst.o execute
>> FAIL: tmpdir-gcc.dg-struct-layout-1/t005 c_compat_x_tst.o- 
>> c_compat_y_tst.o execute
>> FAIL: tmpdir-gcc.dg-struct-layout-1/t006 c_compat_x_tst.o- 
>> c_compat_y_tst.o execute
>> FAIL: tmpdir-gcc.dg-struct-layout-1/t007 c_compat_x_tst.o- 
>> c_compat_y_tst.o execute
>> FAIL: tmpdir-gcc.dg-struct-layout-1/t008 c_compat_x_tst.o- 
>> c_compat_y_tst.o execute
>
> Yeah, I don't think constants are any different here.  One fix might be
> to use simplify_expand_binop instead of extract_bit_field, as per the
> patch below.  The patch also restricts the shifting to forward walks,
> as discussed in the PR trail.
>
> But I'm not sure I understand the fieldmode != BLKmode test in:
>
>       unsigned int backwards = WORDS_BIG_ENDIAN && fieldmode != BLKmode;
>
> Adding it was the (only) net effect of r7985 and r8007,
> but the comment:
>
> 	 However, only do that if the value is not BLKmode.  */
>
> is less than helpful when trying to discern a reason.  Even in those days,
> the code was followed by:
>
>       /* This is the mode we must force value to, so that there will be enough
> 	 subwords to extract.  Note that fieldmode will often (always?) be
> 	 VOIDmode, because that is what store_field uses to indicate that this
> 	 is a bit field, but passing VOIDmode to operand_subword_force will
> 	 result in an abort.  */
>       fieldmode = mode_for_size (nwords * BITS_PER_WORD, MODE_INT, 0);
>
> This latter comment is clearly talking about the value of fieldmode
> before rather than after the call to mode_for_size, so there seems to
> have been a bit of confusion about what fieldmode was actually supposed
> to be (comment says VOIDmode, r8007 says that BLKmode is both possible
> and special).
>
> The pre-r7985 code honoured the comment:
>
>       for (i = 0; i < nwords; i++)
> 	{
> 	  /* If I is 0, use the low-order word in both field and target;
> 	     if I is 1, use the next to lowest word; and so on.  */
>
> And any trimming would always happen in the last iteration (i == nwords - 1);
> in other words, in the high word.
>
> The point of r8007 seems to be that WORDS_BIG_ENDIAN doesn't apply to
> BLKmode fieldmodes, but there must surely be some endianness involved
> somewhere, given that we're extracting words from a multiword value?
>
> This of course predates the public mailing lists by several years.
>
> Anyway, it seems on face value that the original patch for this PR
> (the "experimental fix", which is different from the submitted version,
> but which AIUI avoids the PowerPC and MIPS regressions) is equivalent
> to removing that "&& fieldmode != BLKmode" check on BYTES_BIG_ENDIAN
> == WORDS_BIG_ENDIAN targets.  If that turns out to be correct, I wonder
> what should happen for BYTES_BIG_ENDIAN != WORDS_BIG_ENDIAN?  The only
> two examples I can see are pdp11 and an obselete ARM mode that will be
> removed in 4.8.
>
> So if possible, I'd prefer to revert the original patch for this PR
> and instead adjust the "backwards" test.  I'm just not sure whether
> it should be "WORDS_BIG_ENDIAN" (reverting r8007, which was obviously
> applied for a reason) or something like:
>
>         (fieldmode == BLKmode ? BYTES_BIG_ENDIAN : WORDS_BIG_ENDIAN)

It was decided in the PR trail that we should go with the
simplify_expand_binop patch, so I (reluctantly!) applied the
patch below.

Richard


gcc/
2012-01-17  Andreas Krebbel  <Andreas.Krebbel@de.ibm.com>
	    Richard Sandiford  <rdsandiford@googlemail.com>

	PR middle-end/50325
	PR middle-end/51192
	* optabs.h (simplify_expand_binop): Declare.
	* optabs.c (simplify_expand_binop): Make global.
	* expmed.c (store_bit_field_1): Use simplify_expand_binop on big
	endian targets if the source cannot be exactly covered by word
	mode chunks.

Patch

Index: gcc/optabs.h
===================================================================
--- gcc/optabs.h	2012-01-17 20:31:45.000000000 +0000
+++ gcc/optabs.h	2012-01-17 20:32:00.000000000 +0000
@@ -859,6 +859,10 @@  extern rtx expand_ternary_op (enum machi
 extern rtx expand_binop (enum machine_mode, optab, rtx, rtx, rtx, int,
 			 enum optab_methods);
 
+extern rtx simplify_expand_binop (enum machine_mode mode, optab binoptab,
+				  rtx op0, rtx op1, rtx target, int unsignedp,
+				  enum optab_methods methods);
+
 extern bool force_expand_binop (enum machine_mode, optab, rtx, rtx, rtx, int,
 				enum optab_methods);
 
Index: gcc/optabs.c
===================================================================
--- gcc/optabs.c	2012-01-17 20:31:45.000000000 +0000
+++ gcc/optabs.c	2012-01-17 20:32:00.000000000 +0000
@@ -659,7 +659,7 @@  expand_ternary_op (enum machine_mode mod
    calculated at compile time.  The arguments and return value are
    otherwise the same as for expand_binop.  */
 
-static rtx
+rtx
 simplify_expand_binop (enum machine_mode mode, optab binoptab,
 		       rtx op0, rtx op1, rtx target, int unsignedp,
 		       enum optab_methods methods)
Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c	2012-01-17 20:31:45.000000000 +0000
+++ gcc/expmed.c	2012-01-17 20:32:00.000000000 +0000
@@ -557,9 +557,21 @@  store_bit_field_1 (rtx str_rtx, unsigned
 					    0)
 				     : (int) i * BITS_PER_WORD);
 	  rtx value_word = operand_subword_force (value, wordnum, fieldmode);
+	  unsigned HOST_WIDE_INT new_bitsize =
+	    MIN (BITS_PER_WORD, bitsize - i * BITS_PER_WORD);
 
-	  if (!store_bit_field_1 (op0, MIN (BITS_PER_WORD,
-					    bitsize - i * BITS_PER_WORD),
+	  /* If the remaining chunk doesn't have full wordsize we have
+	     to make sure that for big endian machines the higher order
+	     bits are used.  */
+	  if (new_bitsize < BITS_PER_WORD && BYTES_BIG_ENDIAN && !backwards)
+	    value_word = simplify_expand_binop (word_mode, lshr_optab,
+						value_word,
+						GEN_INT (BITS_PER_WORD
+							 - new_bitsize),
+						NULL_RTX, true,
+						OPTAB_LIB_WIDEN);
+
+	  if (!store_bit_field_1 (op0, new_bitsize,
 				  bitnum + bit_offset,
 				  bitregion_start, bitregion_end,
 				  word_mode,