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)
Richard
gcc/
* optabs.h (simplify_expand_binop): Declare.
* optabs.c (simplify_expand_binop): Make global.
* expmed.c (store_bit_field_1): Use simplify_expand_binop
rather than extract_bit_field to get the high bits of a word.
Only adjust if !backwards.
Index: gcc/optabs.h
===================================================================
--- gcc/optabs.h 2011-11-19 16:44:44.000000000 +0000
+++ gcc/optabs.h 2011-11-19 16:44:58.000000000 +0000
@@ -869,6 +869,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 2011-11-19 16:44:31.000000000 +0000
+++ gcc/optabs.c 2011-11-19 16:44:34.000000000 +0000
@@ -671,7 +671,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 2011-11-19 16:43:26.000000000 +0000
+++ gcc/expmed.c 2011-11-20 09:22:24.000000000 +0000
@@ -564,10 +564,13 @@ store_bit_field_1 (rtx str_rtx, unsigned
/* 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)
- value_word = extract_bit_field (value_word, new_bitsize, 0,
- true, false, NULL_RTX,
- BLKmode, word_mode);
+ 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,
