Tidy store_bit_field_1 & co.

Hi Eric,

Thanks for the review.

Eric Botcazou <ebotcazou@adacore.com> 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),
>> +				 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?

Yeah.  I was also worried that generating an out-of-range subreg seemed
more likely to lead to an ICE-on-invalid than something like a negative
shift would.

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

OK.

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

It's pre-existing.  As you probably guessed, I started out trying to
add an assert that bitnum + bitsize was "sane", but hit a failure
on that particular testcase.  But the same thing happened before
my patch.  The bitnum is in range (24 bits into a 32-bit value),
it's the sum of the two that's bad (40 bits, even though only
32 are available).

Tested in the same way as before.

Richard

gcc/
	* 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.

Tidy store_bit_field_1 & co.

Commit Message

Comments

Patch