Re: Tidy store_bit_field_1 & co.
> I should probably have responded to this earlier, sorry. I'm not sure
> which part you mean, so here's an attempt at justifying the whole block:
>
> 1) WORDS_BIG_ENDIAN is deliberately ignored:
>
> /* The following line once was done only if WORDS_BIG_ENDIAN,
>but I think that is a mistake. WORDS_BIG_ENDIAN is
>meaningful at a much higher level; when structures are copied
>between memory and regs, the higher-numbered regs
>always get higher addresses. */
>
> 2) For MEM:
>
> The old code reached this "if" statement with:
>
> unsigned int unit = (MEM_P (str_rtx)) ? BITS_PER_UNIT : BITS_PER_WORD;
> offset = bitnum / unit;
> bitpos = bitnum % unit;
>
> I.e.:
>
> offset = bitnum / BITS_PER_UNIT;
> bitpos = bitnum % BITS_PER_UNIT;
>
> which the new code does explicitly with:
>
> unsigned HOST_WIDE_INT bitpos = bitnum;
> if (MEM_P (xop0))
> {
> /* Get a reference to the first byte of the field. */
> xop0 = adjust_bitfield_address (xop0, byte_mode,
> bitpos / BITS_PER_UNIT); <-- offset
> bitpos %= BITS_PER_UNIT;
> }
>
> The following:
>
> unit = GET_MODE_BITSIZE (op_mode);
> if (BITS_BIG_ENDIAN != BYTES_BIG_ENDIAN)
> xbitpos = unit - bitsize - xbitpos;
>
> code is the same as before.
>
> 3) For REG, !BYTES_BIG_ENDIAN, !BITS_BIG_ENDIAN:
>
> The easy case. The old code reached this "if" statement with:
>
> unsigned int unit = (MEM_P (str_rtx)) ? BITS_PER_UNIT : BITS_PER_WORD;
> offset = bitnum / unit;
> bitpos = bitnum % unit;
> ...
> if (!MEM_P (op0) && ...)
> {
> ...set op0 to the word containing the field...
> offset = 0;
> }
>
> where the awkward thing is that OFFSET and BITPOS are now out of sync
> with BITNUM. So before the "if" statement:
>
> offset = 0;
> bitpos = bitnum % BITS_PER_WORD;
>
> which if the !MEM_P block above had updated BITNUM too would just
> have been:
>
> offset = 0;
> bitpos = bitnum;
>
> The new code does update BITNUM:
>
> if (!MEM_P (op0) && ...)
> {
> ...set op0 to the word containing the field...
> bitnum %= BITS_PER_WORD;
> }
> ...
> unsigned HOST_WIDE_INT bitpos = bitnum;
>
> so both the following hold:
>
> offset = 0;
> bitpos = bitnum % BITS_PER_WORD;
>
> offset = 0;
> bitpos = bitnum;
>
> The "if" statement doesn't do any endian modification to (X)BITPOS
> before or after the patch.
>
> 4) For REG, !BYTES_BIG_ENDIAN, BITS_BIG_ENDIAN:
>
> Like (3), but at the end we also have:
>
> unit = GET_MODE_BITSIZE (op_mode);
> if (BITS_BIG_ENDIAN != BYTES_BIG_ENDIAN)
> xbitpos = unit - bitsize - xbitpos;
>
> This is the same as before, although the assignment to UNIT has moved
> up a bit.
>
> 5) For REG, BYTES_BIG_ENDIAN, BITS_BIG_ENDIAN:
>
> In this case the code leading up to the "if" statement was:
>
> unsigned int unit = (MEM_P (str_rtx)) ? BITS_PER_UNIT : BITS_PER_WORD;
> offset = bitnum / unit;
> bitpos = bitnum % unit;
> ...
> /* If OP0 is a register, BITPOS must count within a word.
>But as we have it, it counts within whatever size OP0 now has.
>On a bigendian machine, these are not the same, so convert. */
> if (BYTES_BIG_ENDIAN
> && !MEM_P (op0)
> && unit > GET_MODE_BITSIZE (GET_MODE (op0)))
> bitpos += unit - GET_MODE_BITSIZE (GET_MODE (op0));
> ...
> if (!MEM_P (op0) && ...)
> {
> ...set op0 to the word containing the field...
> offset = 0;
> }
>
> So:
>
> offset = 0
> bitpos = bitnum % BITS_PER_WORD;
> if (...original op0 was smaller than a word...)
> bitpos += BITS_PER_WORD - ;
>
> The !MEM_P block narrows OP0s that are wider than a word to word_mode --
> it never narrows more than that -- so this is equivalent to:
>
> offset = 0
> bitpos = bitnum % BITS_PER_WORD;
> if (...current op0 is smaller than a word...)
> bitpos += BITS_PER_WORD - ;
>
> And thanks to the !MEM_P code, op0 is never _wider_ than a word at
> this point, so we have:
>
> offset = 0
> bitpos = bitnum % BITS_PER_WORD + BITS_PER_WORD - ;
>
> Then, inside the "if" statement we did:
>
> /* We have been counting XBITPOS within UNIT.
>Count instead within the size of the register. */
> if (BYTES_BIG_ENDIAN && !MEM_P (xop0))
> xbitpos += GET_MODE_BITSIZE (op_mode) - unit;
>
> where UNIT is still BITS_PER_WORD. So we have:
>
> offset = 0
> bitpos = (bitnum % BITS_PER_WORD + BITS_PER_WORD -
> + - BITS_PER_WORD);
>
> which cancels to:
>
> offset = 0
> bitpos = bitnum % BITS_PER_WORD + ( - );
>
> As above, if the subreg code had updated BITNUM too, this would be
> equivalent to:
>
> offset = 0
> bitpos = bitnum + ( - );
>
> but
Re: Tidy store_bit_field_1 & co.
Eric Botcazou writes:
>> + enum machine_mode op_mode = mode_for_extraction (EP_insv, 3);
>>if (HAVE_insv
>>&& GET_MODE (value) != BLKmode
>>&& bitsize > 0
>> @@ -690,25 +670,34 @@ store_bit_field_1 (rtx str_rtx, unsigned
>> -fstrict-volatile-bitfields is in effect. */
>>&& !(MEM_P (op0) && MEM_VOLATILE_P (op0)
>> && flag_strict_volatile_bitfields > 0)
>> - && ! ((REG_P (op0) || GET_CODE (op0) == SUBREG)
>> -&& (bitsize + bitpos > GET_MODE_BITSIZE (op_mode)))
>>/* Do not use insv if the bit region is restricted and
>> op_mode integer at offset doesn't fit into the
>> restricted region. */
>>&& !(MEM_P (op0) && bitregion_end
>> - && bitnum - bitpos + GET_MODE_BITSIZE (op_mode)
>> + && bitnum - (bitnum % BITS_PER_UNIT) + GET_MODE_BITSIZE (op_mode)
>>
>>> bitregion_end + 1))
>>
>> {
>>struct expand_operand ops[4];
>> - int xbitpos = bitpos;
>> + unsigned HOST_WIDE_INT bitpos = bitnum;
>>rtx value1;
>>rtx xop0 = op0;
>>rtx last = get_last_insn ();
>>bool copy_back = false;
>>
>> - /* Add OFFSET into OP0's address. */
>> + unsigned int unit = GET_MODE_BITSIZE (op_mode);
>>if (MEM_P (xop0))
>> -xop0 = adjust_bitfield_address (xop0, byte_mode, offset);
>> +{
>> + /* Get a reference to the first byte of the field. */
>> + xop0 = adjust_bitfield_address (xop0, byte_mode,
>> + bitpos / BITS_PER_UNIT);
>> + bitpos %= BITS_PER_UNIT;
>> +}
>> + else
>> +{
>> + /* Convert from counting within OP0 to counting in OP_MODE. */
>> + if (BYTES_BIG_ENDIAN)
>> +bitpos += unit - GET_MODE_BITSIZE (GET_MODE (op0));
>> +}
>>
>>/* If xop0 is a register, we need it in OP_MODE
>> to make it acceptable to the format of insv. */
>> @@ -735,20 +724,13 @@ store_bit_field_1 (rtx str_rtx, unsigned
>>copy_back = true;
>> }
>>
>> - /* We have been counting XBITPOS within UNIT.
>> - Count instead within the size of the register. */
>> - if (BYTES_BIG_ENDIAN && !MEM_P (xop0))
>> -xbitpos += GET_MODE_BITSIZE (op_mode) - unit;
>> -
>> - unit = GET_MODE_BITSIZE (op_mode);
>> -
>>/* If BITS_BIG_ENDIAN is zero on a BYTES_BIG_ENDIAN machine, we count
>> "backwards" from the size of the unit we are inserting into. Otherwise, we
>> count bits from the most significant on a
>> BYTES/BITS_BIG_ENDIAN machine. */
>>
>>if (BITS_BIG_ENDIAN != BYTES_BIG_ENDIAN)
>> -xbitpos = unit - bitsize - xbitpos;
>> +bitpos = unit - bitsize - bitpos;
>>
>>/* Convert VALUE to op_mode (which insv insn wants) in VALUE1. */
>>value1 = value;
>
> I guess I see the reasoning, but I cannot say whether it's right or wrong...
I should probably have responded to this earlier, sorry. I'm not sure
which part you mean, so here's an attempt at justifying the whole block:
1) WORDS_BIG_ENDIAN is deliberately ignored:
/* The following line once was done only if WORDS_BIG_ENDIAN,
but I think that is a mistake. WORDS_BIG_ENDIAN is
meaningful at a much higher level; when structures are copied
between memory and regs, the higher-numbered regs
always get higher addresses. */
2) For MEM:
The old code reached this "if" statement with:
unsigned int unit = (MEM_P (str_rtx)) ? BITS_PER_UNIT : BITS_PER_WORD;
offset = bitnum / unit;
bitpos = bitnum % unit;
I.e.:
offset = bitnum / BITS_PER_UNIT;
bitpos = bitnum % BITS_PER_UNIT;
which the new code does explicitly with:
unsigned HOST_WIDE_INT bitpos = bitnum;
if (MEM_P (xop0))
{
/* Get a reference to the first byte of the field. */
xop0 = adjust_bitfield_address (xop0, byte_mode,
bitpos / BITS_PER_UNIT); <-- offset
bitpos %= BITS_PER_UNIT;
}
The following:
unit = GET_MODE_BITSIZE (op_mode);
if (BITS_BIG_ENDIAN != BYTES_BIG_ENDIAN)
xbitpos = unit - bitsize - xbitpos;
code is the same as before.
3) For REG, !BYTES_BIG_ENDIAN, !BITS_BIG_ENDIAN:
The easy case. The old code reached this "if" statement with:
unsigned int unit = (MEM_P (str_rtx)) ? BITS_PER_UNIT : BITS_PER_WORD;
offset = bitnum / unit;
bitpos = bitnum % unit;
...
if (!MEM_P (op0) && ...)
{
...set op0 to the word containing the field...
offset = 0;
}
where the awkward thing is that OFFSET and BITPOS are now out of sync
with BITNUM. So before the "if" statement:
offset = 0;
bitpos = bitnum % BITS_PER_WORD;
which if the !MEM_P block above had updated BITNUM too would just
have been:
offset = 0;
bitpos = bitnum;
The new code does update BITNUM:
if (!MEM_P (op0) && ...)
{
...set op0 to the word containing the field...
Re: Tidy store_bit_field_1 & co.
> * expmed.c (lowpart_bit_field_p): New function.
> (store_bit_field_1): Remove unit, offset, bitpos and byte_offset
> from the outermost scope. Express conditions in terms of bitnum
> rather than offset, bitpos and byte_offset. Split the plain move
> cases into two, one for memory accesses and one for register accesses.
> Allow simplify_gen_subreg to fail rather than calling validate_subreg.
> Move the handling of multiword OP0s after the code that coerces VALUE
> to an integer mode. Use simplify_gen_subreg for this case and assert
> that it succeeds. If the field still spans several words, pass it
> directly to store_split_bit_field. Assume after that point that
> both sources and register targets fit within a word. Replace
> x-prefixed variables with non-prefixed forms. Compute the bitpos
> for insv register operands directly in the chosen unit size, rather
> than going through an intermediate BITS_PER_WORD unit size.
> Update the call to store_fixed_bit_field.
> (store_fixed_bit_field): Replace the bitpos and offset parameters
> with a single bitnum parameter, of the same form as store_bit_field.
> Assume that OP0 contains the full field. Simplify the memory offset
> calculation. Assert that the processed OP0 has an integral mode.
> (store_split_bit_field): Update the call to store_fixed_bit_field.
This looks good to me, modulo:
>/* If the target is a register, overwriting the entire object, or storing
> - a full-word or multi-word field can be done with just a SUBREG. +
> a full-word or multi-word field can be done with just a SUBREG. */ + if
> (!MEM_P (op0)
> + && bitnum % BITS_PER_WORD == 0
> + && bitsize == GET_MODE_BITSIZE (fieldmode)
> + && (bitsize == GET_MODE_BITSIZE (GET_MODE (op0))
> + || bitsize % BITS_PER_WORD == 0))
> +{
> + /* Use the subreg machinery either to narrow OP0 to the required
> + words or to cope with mode punning between equal-sized modes. */
> + rtx sub = simplify_gen_subreg (fieldmode, op0, GET_MODE (op0),
> + bitnum / BITS_PER_UNIT);
> + if (sub)
> + {
> + emit_move_insn (sub, value);
> + return true;
> + }
> +}
Are you sure that you don't need to keep the bitnum == 0 condition in the
first arm that was present in the previous patch? And, on second thoughts,
the first formulation was more in keeping with the comment just above (sorry
about that). So I'd reinstate it and swap the arms:
/* If the target is a register, overwriting the entire object, or storing
a full-word or multi-word field can be done with just a SUBREG. */
if (!MEM_P (op0)
&& bitsize == GET_MODE_BITSIZE (fieldmode)
&& ((bitsize == GET_MODE_BITSIZE (GET_MODE (op0)) && bitnum == 0)
|| (bitsize % BITS_PER_WORD == 0 && bitnum % BITS_PER_WORD == 0)))
{
/* Use the subreg machinery either to narrow OP0 to the required
words or to cope with mode punning between equal-sized modes. */
rtx sub = simplify_gen_subreg (fieldmode, op0, GET_MODE (op0),
bitnum / BITS_PER_UNIT);
if (sub)
{
emit_move_insn (sub, value);
return true;
}
}
In any case, no need to retest, I'd apply it and wait for the apocalypse. :-)
--
Eric Botcazou
Re: Tidy store_bit_field_1 & co.
Hi Eric,
Thanks for the review.
Eric Botcazou writes:
>> -476,34 +468,36 @@ store_bit_field_1 (rtx str_rtx, unsigne
>> }
>>
>>/* If the target is a register, overwriting the entire object, or storing
>> - a full-word or multi-word field can be done with just a SUBREG. +
>> a full-word or multi-word field can be done with just a SUBREG. */ + if
>> (!MEM_P (op0)
>> + && bitsize == GET_MODE_BITSIZE (fieldmode)
>> + && ((bitsize % BITS_PER_WORD == 0
>> + && bitnum % BITS_PER_WORD == 0)
>> + || (bitsize == GET_MODE_BITSIZE (GET_MODE (op0))
>> + && bitnum == 0)))
>> +{
>> + /* Use the subreg machinery either to narrow OP0 to the required
>> + words or to cope with mode punning between equal-sized modes. */
>> + rtx sub = simplify_gen_subreg (fieldmode, op0, GET_MODE (op0),
>> + bitnum / BITS_PER_UNIT);
>> + if (sub)
>> +{
>> + emit_move_insn (sub, value);
>> + return true;
>> +}
>> +}
>
> I'd use the following variant for the sake of symmetry with the MEM_P case:
>
> if (!MEM_P (op0)
> && bitnum % BITS_PER_WORD == 0
> && bitsize == GET_MODE_BITSIZE (fieldmode)
> && [...]
OK.
>> - /* If OP0 is a register, BITPOS must count within a word.
>> - But as we have it, it counts within whatever size OP0 now has.
>> - On a bigendian machine, these are not the same, so convert. */
>> - if (BYTES_BIG_ENDIAN
>> - && !MEM_P (op0)
>> - && unit > GET_MODE_BITSIZE (GET_MODE (op0)))
>> -bitpos += unit - GET_MODE_BITSIZE (GET_MODE (op0));
>> -
>>/* Storing an lsb-aligned field in a register
>> - can be done with a movestrict instruction. */
>> + can be done with a movstrict instruction. */
>>
>>if (!MEM_P (op0)
>> - && (BYTES_BIG_ENDIAN ? bitpos + bitsize == unit : bitpos == 0)
>> + && (BYTES_BIG_ENDIAN
>> + ? bitnum + bitsize == GET_MODE_BITSIZE (GET_MODE (op0))
>> + : bitnum == 0)
>>&& bitsize == GET_MODE_BITSIZE (fieldmode)
>>&& optab_handler (movstrict_optab, fieldmode) != CODE_FOR_nothing)
>> {
>> @@ -558,8 +546,7 @@ store_bit_field_1 (rtx str_rtx, unsigned
>>arg0 = SUBREG_REG (arg0);
>> }
>>
>> - subreg_off = (bitnum % BITS_PER_WORD) / BITS_PER_UNIT
>> - + (offset * UNITS_PER_WORD);
>> + subreg_off = bitnum / BITS_PER_UNIT;
>>if (validate_subreg (fieldmode, GET_MODE (arg0), arg0, subreg_off))
>> {
>>arg0 = gen_rtx_SUBREG (fieldmode, arg0, subreg_off);
>
> Aren't you losing movstrict opportunities in big-endian mode? If GET_MODE
> (op0) is 2 words and bitnum + bitsize == BITS_PER_WORD.
Argh, good catch. I hadn't realised that it could accept lowparts
of individual words.
The revised patch below adds a lowpart_bit_field_p function with the
condition that originally appeared in the extract_bit_field_1 patch.
>> @@ -638,34 +625,6 @@ store_bit_field_1 (rtx str_rtx, unsigned
>>return true;
>> }
>>
>> - /* From here on we can assume that the field to be stored in is
>> - a full-word (whatever type that is), since it is shorter than a word.
>> */ -
>> - /* OFFSET is the number of words or bytes (UNIT says which)
>> - from STR_RTX to the first word or byte containing part of the field.
>> */ -
>> - if (!MEM_P (op0))
>> -{
>> - if (offset != 0
>> - || GET_MODE_SIZE (GET_MODE (op0)) > UNITS_PER_WORD)
>> -{
>> - if (!REG_P (op0))
>> -{
>> - /* Since this is a destination (lvalue), we can't copy
>> - it to a pseudo. We can remove a SUBREG that does not
>> - change the size of the operand. Such a SUBREG may
>> - have been added above. */
>> - gcc_assert (GET_CODE (op0) == SUBREG
>> - && (GET_MODE_SIZE (GET_MODE (op0))
>> - == GET_MODE_SIZE (GET_MODE (SUBREG_REG (op0);
>> - op0 = SUBREG_REG (op0);
>> -}
>> - op0 = gen_rtx_SUBREG (mode_for_size (BITS_PER_WORD, MODE_INT, 0),
>> -op0, (offset * UNITS_PER_WORD));
>> -}
>> - offset = 0;
>> -}
>> -
>>/* If VALUE has a floating-point or complex mode, access it as an
>> integer of the corresponding size. This can occur on a machine
>> with 64 bit registers that uses SFmode for float. It can also
>> @@ -679,9 +638,30 @@ store_bit_field_1 (rtx str_rtx, unsigned
>>emit_move_insn (gen_lowpart (GET_MODE (orig_value), value),
>> orig_value); }
>>
>> - /* Now OFFSET is nonzero only if OP0 is memory
>> - and is therefore always measured in bytes. */
>> + /* If OP0 is a multi-word register, narrow it to the affected word.
>> + If the region spans two words, defer to store_split_bit_field. */
>> + if (!MEM_P (op0) && GET_MODE_SIZE (GET_MODE (op0)) > UNITS_PER_WORD)
>> +{
>> + op0 = simplify_gen_subreg (word_mode, op0, GET_MODE (op0),
>> +
Re: Tidy store_bit_field_1 & co.
> The patch is probably quite hard to review, sorry. I've made the changelog
> a bit more detailed than usual in order to list the individual points.
You meant scary, didn't you? :-)
> * expmed.c (store_bit_field_1): Remove unit, offset, bitpos and
> byte_offset from the outermost scope. Express conditions in terms
> of bitnum rather than offset, bitpos and byte_offset. Split the
> plain move cases into two, one for memory accesses and one for
> register accesses. Allow simplify_gen_subreg to fail rather
> than calling validate_subreg. Move the handling of multiword
> OP0s after the code that coerces VALUE to an integer mode.
> Use simplify_gen_subreg for this case and assert that it succeeds.
> If the field still spans several words, pass it directly to
> store_split_bit_field. Assume after that point that both sources
> and register targets fit within a word. Replace x-prefixed
> variables with non-prefixed forms. Compute the bitpos for insv
> register operands directly in the chosen unit size, rather than
> going through an intermediate BITS_PER_WORD unit size.
> Update the call to store_fixed_bit_field.
> (store_fixed_bit_field): Replace the bitpos and offset parameters
> with a single bitnum parameter, of the same form as store_bit_field.
> Assume that OP0 contains the full field. Simplify the memory offset
> calculation. Assert that the processed OP0 has an integral mode.
> (store_split_bit_field): Update the call to store_fixed_bit_field.
I agree that splitting the memory and register cases is a good idea, as well
as unifying the interface. A few remarks:
> -476,34 +468,36 @@ store_bit_field_1 (rtx str_rtx, unsigne
> }
>
>/* If the target is a register, overwriting the entire object, or storing
> - a full-word or multi-word field can be done with just a SUBREG. +
> a full-word or multi-word field can be done with just a SUBREG. */ + if
> (!MEM_P (op0)
> + && bitsize == GET_MODE_BITSIZE (fieldmode)
> + && ((bitsize % BITS_PER_WORD == 0
> +&& bitnum % BITS_PER_WORD == 0)
> + || (bitsize == GET_MODE_BITSIZE (GET_MODE (op0))
> + && bitnum == 0)))
> +{
> + /* Use the subreg machinery either to narrow OP0 to the required
> + words or to cope with mode punning between equal-sized modes. */
> + rtx sub = simplify_gen_subreg (fieldmode, op0, GET_MODE (op0),
> + bitnum / BITS_PER_UNIT);
> + if (sub)
> + {
> + emit_move_insn (sub, value);
> + return true;
> + }
> +}
I'd use the following variant for the sake of symmetry with the MEM_P case:
if (!MEM_P (op0)
&& bitnum % BITS_PER_WORD == 0
&& bitsize == GET_MODE_BITSIZE (fieldmode)
&& [...]
> - /* If OP0 is a register, BITPOS must count within a word.
> - But as we have it, it counts within whatever size OP0 now has.
> - On a bigendian machine, these are not the same, so convert. */
> - if (BYTES_BIG_ENDIAN
> - && !MEM_P (op0)
> - && unit > GET_MODE_BITSIZE (GET_MODE (op0)))
> -bitpos += unit - GET_MODE_BITSIZE (GET_MODE (op0));
> -
>/* Storing an lsb-aligned field in a register
> - can be done with a movestrict instruction. */
> + can be done with a movstrict instruction. */
>
>if (!MEM_P (op0)
> - && (BYTES_BIG_ENDIAN ? bitpos + bitsize == unit : bitpos == 0)
> + && (BYTES_BIG_ENDIAN
> + ? bitnum + bitsize == GET_MODE_BITSIZE (GET_MODE (op0))
> + : bitnum == 0)
>&& bitsize == GET_MODE_BITSIZE (fieldmode)
>&& optab_handler (movstrict_optab, fieldmode) != CODE_FOR_nothing)
> {
> @@ -558,8 +546,7 @@ store_bit_field_1 (rtx str_rtx, unsigned
> arg0 = SUBREG_REG (arg0);
> }
>
> - subreg_off = (bitnum % BITS_PER_WORD) / BITS_PER_UNIT
> -+ (offset * UNITS_PER_WORD);
> + subreg_off = bitnum / BITS_PER_UNIT;
>if (validate_subreg (fieldmode, GET_MODE (arg0), arg0, subreg_off))
> {
> arg0 = gen_rtx_SUBREG (fieldmode, arg0, subreg_off);
Aren't you losing movstrict opportunities in big-endian mode? If GET_MODE
(op0) is 2 words and bitnum + bitsize == BITS_PER_WORD.
> @@ -638,34 +625,6 @@ store_bit_field_1 (rtx str_rtx, unsigned
>return true;
> }
>
> - /* From here on we can assume that the field to be stored in is
> - a full-word (whatever type that is), since it is shorter than a word.
> */ -
> - /* OFFSET is the number of words or bytes (UNIT says which)
> - from STR_RTX to the first word or byte containing part of the field.
> */ -
> - if (!MEM_P (op0))
> -{
> - if (offset != 0
> - || GET_MODE_SIZE (GET_MODE (op0)) > UNITS_PER_WORD)
> - {
> - if (!REG_P (op0))
> - {
> - /* Since this is a destination (lvalue), we can't copy
> - it to a pseudo. We c
Tidy store_bit_field_1 & co.
insv, extv and extzv have an unusual interface: the structure operand is
supposed to have word_mode if stored in registers or byte_mode if stored
in memory. Andrew's patch to try different insv modes:
http://gcc.gnu.org/ml/gcc-patches/2012-09/msg00126.html
prompted me to try making the patterns more like other optabs.
The use of word and byte "units" for registers and memory respectively
is pretty deeply engrained into the current expand routines, even in the
parts that don't deal directly with the .md patterns. E.g. the bitnum
parameter to store_bit_field always counts from the "leftmost" bit of OP0,
but store_bit_field_1 internally converts it to a trio of "unit",
"offset" (number of whole units) and "bitpos" (position within a unit).
The latter two are then also used in the interface to store_fixed_bit_field,
with the unit being implicit. store_split_bit_field uses the original
"bitnum"-style parameter instead.
This patch makes the code use the original "bitnum" throughout,
and only separate into units where locally useful.
Also, if the field spans two words of a register OP0, store_bit_field_1
reduces OP0 to just the first word. It then makes sure that we fall
through to store_fixed_bit_field, which in turn calls store_split_bit_field,
which knows that OP0 is only partial. I think this is dangerous:
it's the only time that store_bit_field_1 trims OP0 to cover only
part of the field, and so adds another special case for the rest
of the function to handle and ignore. It also makes the interface
to store_fixed_bit_field more complicated.
The patch instead makes store_bit_field_1 call store_split_bit_field
directly where appropriate.
diffstat for this patch and the one I'm about to post says:
expmed.c | 640 +--
1 file changed, 261 insertions(+), 379 deletions(-)
so I'd like to submit them as "clean ups" regardless of whether
I ever get around to the main patterns change.
The patch is probably quite hard to review, sorry. I've made the changelog
a bit more detailed than usual in order to list the individual points.
Tested on x86_64-linux-gnu, powerpc64-linux-gnu, mipsisa64-elf (both -EL
and -EB) and mipsisa32-elf (also both -EL and -EB). OK to install?
Richard
gcc/
* expmed.c (store_bit_field_1): Remove unit, offset, bitpos and
byte_offset from the outermost scope. Express conditions in terms
of bitnum rather than offset, bitpos and byte_offset. Split the
plain move cases into two, one for memory accesses and one for
register accesses. Allow simplify_gen_subreg to fail rather
than calling validate_subreg. Move the handling of multiword
OP0s after the code that coerces VALUE to an integer mode.
Use simplify_gen_subreg for this case and assert that it succeeds.
If the field still spans several words, pass it directly to
store_split_bit_field. Assume after that point that both sources
and register targets fit within a word. Replace x-prefixed
variables with non-prefixed forms. Compute the bitpos for insv
register operands directly in the chosen unit size, rather than
going through an intermediate BITS_PER_WORD unit size.
Update the call to store_fixed_bit_field.
(store_fixed_bit_field): Replace the bitpos and offset parameters
with a single bitnum parameter, of the same form as store_bit_field.
Assume that OP0 contains the full field. Simplify the memory offset
calculation. Assert that the processed OP0 has an integral mode.
(store_split_bit_field): Update the call to store_fixed_bit_field.
Index: gcc/expmed.c
===
--- gcc/expmed.c2012-10-13 19:46:00.862780569 +0100
+++ gcc/expmed.c2012-10-14 11:41:48.692695324 +0100
@@ -49,7 +49,6 @@ static void store_fixed_bit_field (rtx,
unsigned HOST_WIDE_INT,
unsigned HOST_WIDE_INT,
unsigned HOST_WIDE_INT,
- unsigned HOST_WIDE_INT,
rtx);
static void store_split_bit_field (rtx, unsigned HOST_WIDE_INT,
unsigned HOST_WIDE_INT,
@@ -409,15 +408,9 @@ store_bit_field_1 (rtx str_rtx, unsigned
enum machine_mode fieldmode,
rtx value, bool fallback_p)
{
- unsigned int unit
-= (MEM_P (str_rtx)) ? BITS_PER_UNIT : BITS_PER_WORD;
- unsigned HOST_WIDE_INT offset, bitpos;
rtx op0 = str_rtx;
- int byte_offset;
rtx orig_value;
- enum machine_mode op_mode = mode_for_extraction (EP_insv, 3);
-
while (GET_CODE (op0) == SUBREG)
{
/* The following line once was done only if WORDS_BIG_ENDIAN,
@@ -427,8 +420,7 @@ store_bit_field_1 (rtx str_rtx, unsigned
always get higher addresses.
