[5/8] Add narrow_bit_field_mem

This patch splits out a fairly common operation: that of narrowing a MEM
to a particular mode and adjusting the bit number accordingly.

I've kept with "bit_field" rather than "bitfield" for consistency with
the callers, although we do have "bitfield" in "adjust_bitfield_address".

Tested as described in the covering note.  OK to install?

Richard


gcc/
	* expmed.c (narrow_bit_field_mem): New function.
	(store_bit_field_using_insv, store_bit_field_1, store_fixed_bit_field)
	(extract_bit_field_1): Use it.

> This patch splits out a fairly common operation: that of narrowing a MEM
> to a particular mode and adjusting the bit number accordingly.
> 
> I've kept with "bit_field" rather than "bitfield" for consistency with
> the callers, although we do have "bitfield" in "adjust_bitfield_address".

My bad.  Feel free to rename it.

> gcc/
> 	* expmed.c (narrow_bit_field_mem): New function.
> 	(store_bit_field_using_insv, store_bit_field_1, store_fixed_bit_field)
> 	(extract_bit_field_1): Use it.

This is a good cleanup but...

> Index: gcc/expmed.c
> ===================================================================
> --- gcc/expmed.c	2012-10-30 19:25:44.797368678 +0000
> +++ gcc/expmed.c	2012-10-30 19:25:47.730368671 +0000
> @@ -387,6 +387,23 @@ mode_for_extraction (enum extraction_pat
>    return data->operand[opno].mode;
>  }
> 
> +/* Adjust bitfield memory MEM so that it points to the first unit of
> +   mode MODE that contains the bitfield at bit position BITNUM.
> +   Set NEW_BITNUM to the bit position of the field within the
> +   new memory.  */
> +
> +static rtx
> +narrow_bit_field_mem (rtx mem, enum machine_mode mode,
> +		      unsigned HOST_WIDE_INT bitnum,
> +		      unsigned HOST_WIDE_INT &new_bitnum)
> +{
> +  unsigned int unit = GET_MODE_BITSIZE (mode);
> +  unsigned HOST_WIDE_INT offset = bitnum / unit * GET_MODE_SIZE (mode);
> +  mem = adjust_bitfield_address (mem, mode, offset);
> +  new_bitnum = bitnum % unit;
> +  return mem;
> +}

Ugh. :-)  I cannot see any advantages in using references here except for...

> +    /* Get a reference to the first byte of the field.  */
> +    xop0 = narrow_bit_field_mem (xop0, byte_mode, bitnum, bitnum);

> +    op0 = narrow_bit_field_mem (op0, byte_mode, bitnum, bitnum);

... mightily confusing the reader here.

Eric Botcazou <ebotcazou@adacore.com> writes:
>> This patch splits out a fairly common operation: that of narrowing a MEM
>> to a particular mode and adjusting the bit number accordingly.
>> 
>> I've kept with "bit_field" rather than "bitfield" for consistency with
>> the callers, although we do have "bitfield" in "adjust_bitfield_address".
>
> My bad.  Feel free to rename it.

TBH, I prefer "bitfield", so I'll leave it be :-)

>> gcc/
>> 	* expmed.c (narrow_bit_field_mem): New function.
>> 	(store_bit_field_using_insv, store_bit_field_1, store_fixed_bit_field)
>> 	(extract_bit_field_1): Use it.
>
> This is a good cleanup but...
>
>> Index: gcc/expmed.c
>> ===================================================================
>> --- gcc/expmed.c	2012-10-30 19:25:44.797368678 +0000
>> +++ gcc/expmed.c	2012-10-30 19:25:47.730368671 +0000
>> @@ -387,6 +387,23 @@ mode_for_extraction (enum extraction_pat
>>    return data->operand[opno].mode;
>>  }
>> 
>> +/* Adjust bitfield memory MEM so that it points to the first unit of
>> +   mode MODE that contains the bitfield at bit position BITNUM.
>> +   Set NEW_BITNUM to the bit position of the field within the
>> +   new memory.  */
>> +
>> +static rtx
>> +narrow_bit_field_mem (rtx mem, enum machine_mode mode,
>> +		      unsigned HOST_WIDE_INT bitnum,
>> +		      unsigned HOST_WIDE_INT &new_bitnum)
>> +{
>> +  unsigned int unit = GET_MODE_BITSIZE (mode);
>> +  unsigned HOST_WIDE_INT offset = bitnum / unit * GET_MODE_SIZE (mode);
>> +  mem = adjust_bitfield_address (mem, mode, offset);
>> +  new_bitnum = bitnum % unit;
>> +  return mem;
>> +}
>
> Ugh. :-)  I cannot see any advantages in using references here except for...
>
>> +    /* Get a reference to the first byte of the field.  */
>> +    xop0 = narrow_bit_field_mem (xop0, byte_mode, bitnum, bitnum);
>
>> +    op0 = narrow_bit_field_mem (op0, byte_mode, bitnum, bitnum);
>
> ... mightily confusing the reader here.

Would it be OK with a pointer, but keeping the interface the same?
That's certainly fine by me.

That's one of the things I'm not sure about after the C++ conversion:
I've noticed some references creep in, but when should we use references
and when pointers?  I think Richard B made a comment about using references
for things that can't be null.

Richard

> Would it be OK with a pointer, but keeping the interface the same?
> That's certainly fine by me.

Yes, a pointer would make things more legible here.

> That's one of the things I'm not sure about after the C++ conversion:
> I've noticed some references creep in, but when should we use references
> and when pointers?  I think Richard B made a comment about using references
> for things that can't be null.

I'm personally dubious about using references, especially in the middle-end of 
a file where all other functions use pointers; consistency should come first.

On Wed, Oct 31, 2012 at 9:16 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Would it be OK with a pointer, but keeping the interface the same?
>> That's certainly fine by me.
>
> Yes, a pointer would make things more legible here.
>
>> That's one of the things I'm not sure about after the C++ conversion:
>> I've noticed some references creep in, but when should we use references
>> and when pointers?  I think Richard B made a comment about using references
>> for things that can't be null.
>
> I'm personally dubious about using references, especially in the middle-end of
> a file where all other functions use pointers; consistency should come first.

I agree.  My comment was for isolated code parts that are being rewritten
(I think it was the wide-int class).  Consistency comes first.

Thanks,
Richard.

> --
> Eric Botcazou

On Oct 31, 2012, at 2:45 AM, Richard Biener <richard.guenther@gmail.com> wrote:.
> My comment was for isolated code parts that are being rewritten
> (I think it was the wide-int class).  Consistency comes first.

In the case of wide int, we only use references in one very narrow way.  We use const T& as parameters instead of T, to avoid the copy of a large piece of data.  Otherwise, we use value semantic for the entire interface and we only do this optimization for one class, wide_int.  There is one other use, and that is for the logical operator [], which returns something that can be modified, again, this is for the cleanliness of the interface.  Since the entire interface is self consistent, I think we meet the goal of consistency.  One benefit of this interface choice is that we can support things like a+b, directly.  If we have used pointers, the natural a+1 would not be the value of a plus 1, but rather the nonexistent object that would be in memory after a, which isn't what we want.