Eric Botcazou <ebotca...@adacore.com> 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...
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 - <bits in op0>;
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 - <bits in op0>;
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 - <bits in op0>;
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 in op0>
+ <bits in op_mode> - BITS_PER_WORD);
which cancels to:
offset = 0
bitpos = bitnum % BITS_PER_WORD + (<bits in op_mode> - <bits in op0>);
As above, if the subreg code had updated BITNUM too, this would be
equivalent to:
offset = 0
bitpos = bitnum + (<bits in op_mode> - <bits in op0>);
but isn't as things stand. The new code does update BITNUM:
if (!MEM_P (op0) && ...)
{
...set op0 to the word containing the field...
bitnum %= BITS_PER_WORD;
}
and then calculates the final BITPOS directly:
unsigned HOST_WIDE_INT bitpos = bitnum;
unsigned int unit = GET_MODE_BITSIZE (op_mode);
if (MEM_P (xop0))
...
else
{
/* Convert from counting within OP0 to counting in OP_MODE. */
if (BYTES_BIG_ENDIAN)
bitpos += unit - GET_MODE_BITSIZE (GET_MODE (op0));
}
so both the following hold:
offset = 0
bitpos = bitnum % BITS_PER_WORD + (<bits in op_mode> - <bits in op0>);
offset = 0
bitpos = bitnum + (<bits in op_mode> - <bits in op0>);
6) For REG, BYTES_BIG_ENDIAN, !BITS_BIG_ENDIAN:
Like (5) but 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, except that the UNIT assignment became
part of (5).
Hope I've got that right...
Of course, the idea is that the new code should make sense from first
principles. This was just trying to show that it isn't supposed to have
changed the behaviour.
Richard
