Patchwork [5/8] Add narrow_bit_field_mem

login
register
mail settings
Submitter Richard Sandiford
Date Oct. 30, 2012, 7:27 p.m.
Message ID <87wqy7hiab.fsf@talisman.home>
Download mbox | patch
Permalink /patch/195610/
State New
Headers show

Comments

Richard Sandiford - Oct. 30, 2012, 7:27 p.m.
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.
Eric Botcazou - Oct. 30, 2012, 10:31 p.m.
> 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.
Richard Sandiford - Oct. 31, 2012, 7:56 a.m.
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
Eric Botcazou - Oct. 31, 2012, 8:16 a.m.
> 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.
Richard Guenther - Oct. 31, 2012, 9:45 a.m.
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
Mike Stump - Oct. 31, 2012, 5:51 p.m.
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.

Patch

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;
+}
+
 /* Return true if a bitfield of size BITSIZE at bit number BITNUM within
    a structure of mode STRUCT_MODE represents a lowpart subreg.   The subreg
    offset is then BITNUM / BITS_PER_UNIT.  */
@@ -424,11 +441,8 @@  store_bit_field_using_insv (rtx op0, uns
     return false;
 
   if (MEM_P (xop0))
-    {
-      /* Get a reference to the first byte of the field.  */
-      xop0 = adjust_bitfield_address (xop0, byte_mode, bitnum / BITS_PER_UNIT);
-      bitnum %= BITS_PER_UNIT;
-    }
+    /* Get a reference to the first byte of the field.  */
+    xop0 = narrow_bit_field_mem (xop0, byte_mode, bitnum, bitnum);
   else
     {
       /* Convert from counting within OP0 to counting in OP_MODE.  */
@@ -831,18 +845,14 @@  store_bit_field_1 (rtx str_rtx, unsigned
 	       && GET_MODE_BITSIZE (bestmode) > MEM_ALIGN (op0)))
 	{
 	  rtx last, tempreg, xop0;
-	  unsigned int unit;
-	  unsigned HOST_WIDE_INT offset, bitpos;
+	  unsigned HOST_WIDE_INT bitpos;
 
 	  last = get_last_insn ();
 
 	  /* Adjust address to point to the containing unit of
 	     that mode.  Compute the offset as a multiple of this unit,
 	     counting in bytes.  */
-	  unit = GET_MODE_BITSIZE (bestmode);
-	  offset = (bitnum / unit) * GET_MODE_SIZE (bestmode);
-	  bitpos = bitnum % unit;
-	  xop0 = adjust_bitfield_address (op0, bestmode, offset);
+	  xop0 = narrow_bit_field_mem (op0, bestmode, bitnum, bitpos);
 
 	  /* Fetch that unit, store the bitfield in it, then store
 	     the unit.  */
@@ -975,9 +985,7 @@  store_fixed_bit_field (rtx op0, unsigned
 	  return;
 	}
 
-      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;
+      op0 = narrow_bit_field_mem (op0, mode, bitnum, bitnum);
     }
 
   mode = GET_MODE (op0);
@@ -1246,11 +1254,8 @@  extract_bit_field_using_extv (rtx op0, u
     return NULL_RTX;
 
   if (MEM_P (op0))
-    {
-      /* Get a reference to the first byte of the field.  */
-      op0 = adjust_bitfield_address (op0, byte_mode, bitnum / BITS_PER_UNIT);
-      bitnum %= BITS_PER_UNIT;
-    }
+    /* Get a reference to the first byte of the field.  */
+    op0 = narrow_bit_field_mem (op0, byte_mode, bitnum, bitnum);
   else
     {
       /* Convert from counting within OP0 to counting in EXT_MODE.  */
@@ -1640,23 +1645,19 @@  extract_bit_field_1 (rtx str_rtx, unsign
 	  && !(SLOW_UNALIGNED_ACCESS (bestmode, MEM_ALIGN (op0))
 	       && GET_MODE_BITSIZE (bestmode) > MEM_ALIGN (op0)))
 	{
-	  unsigned HOST_WIDE_INT offset, bitpos;
-
-	  /* Compute the offset as a multiple of this unit,
-	     counting in bytes.  */
-	  unsigned int unit = GET_MODE_BITSIZE (bestmode);
-	  offset = (bitnum / unit) * GET_MODE_SIZE (bestmode);
-	  bitpos = bitnum % unit;
+	  unsigned HOST_WIDE_INT bitpos;
+	  rtx xop0 = narrow_bit_field_mem (op0, bestmode, bitnum, bitpos);
 
-	  /* Make sure the register is big enough for the whole field.  */
-	  if (bitpos + bitsize <= unit)
+	  /* Make sure the register is big enough for the whole field.
+	     (It might not be if bestmode == GET_MODE (op0) and the input
+	     code was invalid.)  */
+	  if (bitpos + bitsize <= GET_MODE_BITSIZE (bestmode))
 	    {
-	      rtx last, result, xop0;
+	      rtx last, result;
 
 	      last = get_last_insn ();
 
 	      /* Fetch it to a register in that size.  */
-	      xop0 = adjust_bitfield_address (op0, bestmode, offset);
 	      xop0 = force_reg (bestmode, xop0);
 	      result = extract_bit_field_1 (xop0, bitsize, bitpos,
 					    unsignedp, packedp, target,