Richard Sandiford <rdsandif...@googlemail.com> writes:
> Iain Sandoe <develo...@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.kreb...@de.ibm.com>
Richard Sandiford <rdsandif...@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.
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,
