Re: Tidy store_bit_field_1 & co.

[Eric Botcazou <ebotcazou@adacore.com>]

> 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.

Thanks for the explanation (or rather the proof :-).  No objections by me.

-- 
Eric Botcazou