Re: [5/8] Add narrow_bit_field_mem

2012-10-31 Thread Richard Sandiford 
Eric Botcazou ebotca...@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 +
 +++ gcc/expmed.c 2012-10-30 19:25:47.730368671 +
 @@ -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

Re: [5/8] Add narrow_bit_field_mem

2012-10-31 Thread Eric Botcazou 
 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.

-- 
Eric Botcazou

Re: [5/8] Add narrow_bit_field_mem

2012-10-31 Thread Richard Biener 
On Wed, Oct 31, 2012 at 9:16 AM, Eric Botcazou ebotca...@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

Re: [5/8] Add narrow_bit_field_mem

2012-10-31 Thread Mike Stump 
On Oct 31, 2012, at 2:45 AM, Richard Biener richard.guent...@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.

[5/8] Add narrow_bit_field_mem

2012-10-30 Thread Richard Sandiford 
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.

Index: gcc/expmed.c
===
--- gcc/expmed.c2012-10-30 19:25:44.797368678 +
+++ gcc/expmed.c2012-10-30 19:25:47.730368671 +
@@ -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;

Re: [5/8] Add narrow_bit_field_mem

2012-10-30 Thread Eric Botcazou 
 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 +
 +++ gcc/expmed.c  2012-10-30 19:25:47.730368671 +
 @@ -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