> 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 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),
> + bitnum / BITS_PER_WORD * UNITS_PER_WORD);
> + gcc_assert (op0);
> + bitnum %= BITS_PER_WORD;
> + if (bitnum + bitsize > BITS_PER_WORD)
> + {
> + if (!fallback_p)
> + return false;
> +
> + store_split_bit_field (op0, bitsize, bitnum, bitregion_start,
> + bitregion_end, value);
> + return true;
> + }
> + }
> +
> + /* From here on we can assume that the field to be stored in fits
> + within a word. If the destination is a register, it too fits
> + in a word. */
I presume the reasoning is that the offset != 0 test is redundant with the
test on the mode size because of the REG_P && ... early exit?
> + 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...
> - total_bits = GET_MODE_BITSIZE (mode);
> -
> - /* Make sure bitpos is valid for the chosen mode. Adjust BITPOS to
> - be in the range 0 to total_bits-1, and put any excess bytes in
> - OFFSET. */
> - if (bitpos >= total_bits)
> - {
> - offset += (bitpos / total_bits) * (total_bits / BITS_PER_UNIT);
> - bitpos -= ((bitpos / total_bits) * (total_bits / BITS_PER_UNIT)
> - * BITS_PER_UNIT);
> - }
> -
> - /* Get ref to an aligned byte, halfword, or word containing the
> field. - Adjust BITPOS to be position within a word,
> - and OFFSET to be the offset of that word.
> - Then alter OP0 to refer to that word. */
> - bitpos += (offset % (total_bits / BITS_PER_UNIT)) * BITS_PER_UNIT;
> - offset -= (offset % (total_bits / BITS_PER_UNIT));
> - op0 = adjust_bitfield_address (op0, mode, offset);
> + HOST_WIDE_INT bit_offset = bitnum - bitnum % GET_MODE_BITSIZE (mode);
> + op0 = adjust_bitfield_address (op0, mode, bit_offset /
> BITS_PER_UNIT); + bitnum -= bit_offset;
> }
Gorgeous.
> mode = GET_MODE (op0);
> -
> - /* Now MODE is either some integral mode for a MEM as OP0,
> - or is a full-word for a REG as OP0. TOTAL_BITS corresponds.
> - The bit field is contained entirely within OP0.
> - BITPOS is the starting bit number within OP0.
> - (OP0's mode may actually be narrower than MODE.) */
> + gcc_assert (SCALAR_INT_MODE_P (mode));
> + /* Note that bitsize + bitnum can be greater than GET_MODE_BITSIZE (mode)
> + for invalid input, such as f5 from gcc.dg/pr48335-2.c. */
Blank line after the assertion.
> if (BYTES_BIG_ENDIAN)
> - /* BITPOS is the distance between our msb
> - and that of the containing datum.
> - Convert it to the distance from the lsb. */
> - bitpos = total_bits - bitsize - bitpos;
> + /* BITNUM is the distance between our msb
> + and that of the containing datum.
> + Convert it to the distance from the lsb. */
> + bitnum = GET_MODE_BITSIZE (mode) - bitsize - bitnum;
So bitnum can be negative? Is that new or pre-existing?
--
Eric Botcazou
