Patchwork [4/8] Add bit_field_mode_iterator

login
register
mail settings
Submitter Richard Sandiford
Date Nov. 3, 2012, 11:21 a.m.
Message ID <87390rexub.fsf@talisman.home>
Download mbox | patch
Permalink /patch/196822/
State New
Headers show

Comments

Richard Sandiford - Nov. 3, 2012, 11:21 a.m.
get_best_mode has various checks to decide what counts as an acceptable
bitfield mode.  It actually has two copies of them, with slightly different
alignment checks:

  MIN (unit, BIGGEST_ALIGNMENT) > align

vs.

  unit <= MIN (align, BIGGEST_ALIGNMENT)

The second looks more correct, since we can't necessarily guarantee
larger alignments than BIGGEST_ALIGNMENT in all cases.

This patch adds a new iterator class that can be used to walk through
the modes, and rewrites get_best_mode to use it.  I kept the existing
checks with two changes:

- bitregion_start is now tested independently of bitregion_end
- MAX_FIXED_MODE_SIZE is used as a limit even if a bitregion is defined

It shouldn't make any difference in practice, but both changes felt
more in keeping with the documentation of bitregion_start and
MAX_FIXED_MODE_SIZE, and the next patch wants the bitregion_end
test to be separate from bitregion_start.

The behaviour of the Sequent i386 compiler probably isn't the
issue it once was, but that's also dealt with in the next patch.

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

Richard


gcc/
	* machmode.h (bit_field_mode_iterator): New class.
	(get_best_mode): Change final parameter to bool.
	* stor-layout.c (bit_field_mode_iterator::bit_field_mode_iterator)
	(bit_field_mode_iterator::next_mode): New functions, split out from...
	(get_best_mode): ...here.  Change final parameter to bool.
	Use bit_field_mode_iterator.
Eric Botcazou - Nov. 13, 2012, 12:41 p.m.
> get_best_mode has various checks to decide what counts as an acceptable
> bitfield mode.  It actually has two copies of them, with slightly different
> alignment checks:
> 
>   MIN (unit, BIGGEST_ALIGNMENT) > align
> 
> vs.
> 
>   unit <= MIN (align, BIGGEST_ALIGNMENT)
> 
> The second looks more correct, since we can't necessarily guarantee
> larger alignments than BIGGEST_ALIGNMENT in all cases.

Under the assumption that integer modes really require maximal alignment, i.e. 
whatever BIGGEST_ALIGNMENT is, I agree.

> This patch adds a new iterator class that can be used to walk through
> the modes, and rewrites get_best_mode to use it.  I kept the existing
> checks with two changes:
> 
> - bitregion_start is now tested independently of bitregion_end

The comments needs to be updated then.

> - MAX_FIXED_MODE_SIZE is used as a limit even if a bitregion is defined

This makes sense I think.

> It shouldn't make any difference in practice, but both changes felt
> more in keeping with the documentation of bitregion_start and
> MAX_FIXED_MODE_SIZE, and the next patch wants the bitregion_end
> test to be separate from bitregion_start.
> 
> The behaviour of the Sequent i386 compiler probably isn't the
> issue it once was, but that's also dealt with in the next patch.
> 
> Tested as described in the covering note.  OK to install?
> 
> Richard
> 
> 
> gcc/
> 	* machmode.h (bit_field_mode_iterator): New class.
> 	(get_best_mode): Change final parameter to bool.
> 	* stor-layout.c (bit_field_mode_iterator::bit_field_mode_iterator)
> 	(bit_field_mode_iterator::next_mode): New functions, split out from...
> 	(get_best_mode): ...here.  Change final parameter to bool.
> 	Use bit_field_mode_iterator.

This looks good to me, modulo:

> +  volatilep_ (volatilep), count_ (0)
> +{
> +  if (bitregion_end_)
> +    bitregion_end_ += 1;
> +}

IMO this is confusing.  I think bitregion_end/bitregion_end_ should have a 
consistent meaning.


> +/* Calls to this function return successively larger modes that can be used
> +   to represent the bitfield.  Return true if another bitfield mode is +  
> available, storing it in *OUT_MODE if so.  */
> +
> +bool bit_field_mode_iterator::next_mode (enum machine_mode *out_mode)

'bool' on its own line I think.


I find the interface a bit awkward though.  Can't we model it on the existing 
iterators in basic-block.h or tree-flow.h?  get_best_mode would be written:

  FOR_EACH_BITFIELD_MODE (mode, iter, bitsize, bitpos,
			  bitregion_start, bitregion_end,
			  align, volatilep)
    {
	if (largest_mode != VOIDmode
	    && GET_MODE_SIZE (mode) > GET_MODE_SIZE (largest_mode)
	  break;

	if (iter.prefer_smaller_modes ())
	  return mode;

	widest_mode = mode; 
    }

  return widest_mode;

and the implementation entirely hidden.
Richard Henderson - Nov. 13, 2012, 9:46 p.m.
On 11/13/2012 04:41 AM, Eric Botcazou wrote:
> I find the interface a bit awkward though.  Can't we model it on the existing 
> iterators in basic-block.h or tree-flow.h?  get_best_mode would be written:
> 
>   FOR_EACH_BITFIELD_MODE (mode, iter, bitsize, bitpos,
> 			  bitregion_start, bitregion_end,
> 			  align, volatilep)

Now that we're in C++, I think we should be using iterators that are much
more in the style of libstdc++.  I agree that the .next interface used here
is a bit confusing.

I'd expect to see something like

  for (bit_field_mode_iterator i(x,y,z); i ; ++i)
    {
      enum machine_mode mode = *i;
      ...
    }

For 4.9, I expect that someone can slowly purge our FOR_EACH_* macros...


r~
Eric Botcazou - Nov. 13, 2012, 10:03 p.m.
> Now that we're in C++, I think we should be using iterators that are much
> more in the style of libstdc++.  I agree that the .next interface used here
> is a bit confusing.
> 
> I'd expect to see something like
> 
>   for (bit_field_mode_iterator i(x,y,z); i ; ++i)
>     {
>       enum machine_mode mode = *i;
>       ...
>     }

I pondered on that for half an hour. :-)  But the amount of stuff you need to 
write to make it work in this particular case will make the implementation 
convoluted and bloated for no obvious gains IMO.
Richard Sandiford - Nov. 15, 2012, 12:10 p.m.
Eric Botcazou <ebotcazou@adacore.com> writes:
>> Now that we're in C++, I think we should be using iterators that are much
>> more in the style of libstdc++.  I agree that the .next interface used here
>> is a bit confusing.
>> 
>> I'd expect to see something like
>> 
>>   for (bit_field_mode_iterator i(x,y,z); i ; ++i)
>>     {
>>       enum machine_mode mode = *i;
>>       ...
>>     }
>
> I pondered on that for half an hour. :-)  But the amount of stuff you need to 
> write to make it work in this particular case will make the implementation 
> convoluted and bloated for no obvious gains IMO.

Yeah, I'd also thought about doing it as a "proper" iterator, and came
to the same conclusion.  Although I was thinking of:

  bit_field_def def (blah...);
  for (bit_field_def::mode_iterator i = def.modes_begin();
       i != def.modes_end();
       ++i)
    {
      enum machine_mode mode = *i;
      ...
    }

Then presumably we'd need all the other iterator gloss too
(value_type, etc.).

I hadn't thought about an operator bool terminator.  I agree that's
probably simpler, but do any libstdc++ classes have the same thing?
It doesn't feel any more standard than the "while (get_more)" idiom to me,
but that's probably just my ignorance of C++.

My problem with:

  FOR_EACH_BITFIELD_MODE (mode, iter, bitsize, bitpos,
			  bitregion_start, bitregion_end,
			  align, volatilep)

is that there are too many parameters to make it obvious what we're
actually iterating over.  I.e. these FOR_EACH_* macros tend to have a
combination of three things: inputs, "internal" iterator state that
nevertheless needs to be defined separately in order to avoid surprising
name clashes, and the iterator variable itself.  When there's a lot of
parameters, it's not always obvious which is which.

Also, it makes it harder to implement loops like:

  setup
  if (get first mode)
    if (want to search for more)
      while (more modes...)

which is the structure used in patch 7.

Admittedly both "problems" apply to some of the existing FOR_EACH_*
macros too, so there's definitely a consistency argument in favour of
doing this anyway.

"next" was supposed to be "find and return another mode" rather than "++".
Did you think it was confusing because "next" sounded too much like the latter?
Or was it the whole "while (get more)" thing you didn't like?

Maybe it would be better to forget about using classes here and have
something like (with "bit_field_def def"):

    mode = smallest_bit_field_mode (&def, params...);
    while (mode != MAX_MACHINE_MODE)
      {
        ...
        mode = wider_bit_field_mode (&def, mode);
      }

Richard
Richard Henderson - Nov. 15, 2012, 8:39 p.m.
On 11/15/2012 04:10 AM, Richard Sandiford wrote:
> "next" was supposed to be "find and return another mode" rather than "++".
> Did you think it was confusing because "next" sounded too much like the latter?

I wasn't keen on "next" being find-and-return, though I didn't
actually find it confusing.  And perhaps rather than bikeshed
this too much now, we should table this for revision in 4.9...

> I hadn't thought about an operator bool terminator.  I agree that's
> probably simpler, but do any libstdc++ classes have the same thing?
> It doesn't feel any more standard than the "while (get_more)" idiom to me,
> but that's probably just my ignorance of C++.

... when we can attack all the iterators.

No, you're right that operator bool as a terminator isn't standard.
Though for many purposes it seems better than the "!= fake_end_object"
semantics that we'd have to use otherwise.

That's a discussion that we should have generally as we find our 
feet with C++ in GCC.

Unless Eric has any strong objections, I think this patch is ok.
And thus the entire patch set, as I havn't seen anything else that
raises a red flag.


r~
Richard Sandiford - Nov. 18, 2012, 5:34 p.m.
Richard Henderson <rth@redhat.com> writes:
> On 11/15/2012 04:10 AM, Richard Sandiford wrote:
>> "next" was supposed to be "find and return another mode" rather than "++".
>> Did you think it was confusing because "next" sounded too much like
>> the latter?
>
> I wasn't keen on "next" being find-and-return, though I didn't
> actually find it confusing.  And perhaps rather than bikeshed
> this too much now, we should table this for revision in 4.9...
>
>> I hadn't thought about an operator bool terminator.  I agree that's
>> probably simpler, but do any libstdc++ classes have the same thing?
>> It doesn't feel any more standard than the "while (get_more)" idiom to me,
>> but that's probably just my ignorance of C++.
>
> ... when we can attack all the iterators.
>
> No, you're right that operator bool as a terminator isn't standard.
> Though for many purposes it seems better than the "!= fake_end_object"
> semantics that we'd have to use otherwise.
>
> That's a discussion that we should have generally as we find our 
> feet with C++ in GCC.
>
> Unless Eric has any strong objections, I think this patch is ok.
> And thus the entire patch set, as I havn't seen anything else that
> raises a red flag.

Thanks.  Committed with the changes Eric asked for after retesting
on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf.

Richard

Patch

Index: gcc/machmode.h
===================================================================
--- gcc/machmode.h	2012-11-02 22:43:15.633373159 +0000
+++ gcc/machmode.h	2012-11-02 22:47:03.291372600 +0000
@@ -259,13 +259,36 @@  extern enum machine_mode int_mode_for_mo
 
 extern enum machine_mode mode_for_vector (enum machine_mode, unsigned);
 
+/* A class for iterating through possible bitfield modes.  */
+class bit_field_mode_iterator
+{
+public:
+  bit_field_mode_iterator (HOST_WIDE_INT, HOST_WIDE_INT,
+			   HOST_WIDE_INT, HOST_WIDE_INT,
+			   unsigned int, bool);
+  bool next_mode (enum machine_mode *);
+  bool prefer_smaller_modes ();
+
+private:
+  enum machine_mode mode_;
+  /* We use signed values here because the bit position can be negative
+     for invalid input such as gcc.dg/pr48335-8.c.  */
+  HOST_WIDE_INT bitsize_;
+  HOST_WIDE_INT bitpos_;
+  HOST_WIDE_INT bitregion_start_;
+  HOST_WIDE_INT bitregion_end_;
+  unsigned int align_;
+  bool volatilep_;
+  int count_;
+};
+
 /* Find the best mode to use to access a bit field.  */
 
 extern enum machine_mode get_best_mode (int, int,
 					unsigned HOST_WIDE_INT,
 					unsigned HOST_WIDE_INT,
 					unsigned int,
-					enum machine_mode, int);
+					enum machine_mode, bool);
 
 /* Determine alignment, 1<=result<=BIGGEST_ALIGNMENT.  */
 
Index: gcc/stor-layout.c
===================================================================
--- gcc/stor-layout.c	2012-11-02 22:43:15.633373159 +0000
+++ gcc/stor-layout.c	2012-11-02 22:47:11.367372581 +0000
@@ -2624,6 +2624,98 @@  fixup_unsigned_type (tree type)
   layout_type (type);
 }
 
+/* Construct an iterator for a bitfield that spans BITSIZE bits,
+   starting at BITPOS.
+
+   BITREGION_START is the bit position of the first bit in this
+   sequence of bit fields.  BITREGION_END is the last bit in this
+   sequence.  If these two fields are non-zero, we should restrict the
+   memory access to a maximum sized chunk of
+   BITREGION_END - BITREGION_START + 1.  Otherwise, we are allowed to touch
+   any adjacent non bit-fields.
+
+   ALIGN is the alignment of the underlying object in bits.
+   VOLATILEP says whether the bitfield is volatile.  */
+
+bit_field_mode_iterator
+::bit_field_mode_iterator (HOST_WIDE_INT bitsize, HOST_WIDE_INT bitpos,
+			   HOST_WIDE_INT bitregion_start,
+			   HOST_WIDE_INT bitregion_end,
+			   unsigned int align, bool volatilep)
+: mode_ (GET_CLASS_NARROWEST_MODE (MODE_INT)), bitsize_ (bitsize),
+  bitpos_ (bitpos), bitregion_start_ (bitregion_start),
+  bitregion_end_ (bitregion_end), align_ (MIN (align, BIGGEST_ALIGNMENT)),
+  volatilep_ (volatilep), count_ (0)
+{
+  if (bitregion_end_)
+    bitregion_end_ += 1;
+}
+
+/* Calls to this function return successively larger modes that can be used
+   to represent the bitfield.  Return true if another bitfield mode is
+   available, storing it in *OUT_MODE if so.  */
+
+bool bit_field_mode_iterator::next_mode (enum machine_mode *out_mode)
+{
+  for (; mode_ != VOIDmode; mode_ = GET_MODE_WIDER_MODE (mode_))
+    {
+      unsigned int unit = GET_MODE_BITSIZE (mode_);
+
+      /* Skip modes that don't have full precision.  */
+      if (unit != GET_MODE_PRECISION (mode_))
+	continue;
+
+      /* Skip modes that are too small.  */
+      if ((bitpos_ % unit) + bitsize_ > unit)
+	continue;
+
+      /* Stop if the mode is too wide to handle efficiently.  */
+      if (unit > MAX_FIXED_MODE_SIZE)
+	break;
+
+      /* Don't deliver more than one multiword mode; the smallest one
+	 should be used.  */
+      if (count_ > 0 && unit > BITS_PER_WORD)
+	break;
+
+      /* Stop if the mode is wider than the alignment of the containing
+	 object.
+
+	 It is tempting to omit the following line unless STRICT_ALIGNMENT
+	 is true.  But that is incorrect, since if the bitfield uses part
+	 of 3 bytes and we use a 4-byte mode, we could get a spurious segv
+	 if the extra 4th byte is past the end of memory.
+	 (Though at least one Unix compiler ignores this problem:
+	 that on the Sequent 386 machine.  */
+      if (unit > align_)
+	break;
+
+      /* Stop if the mode goes outside the bitregion.  */
+      HOST_WIDE_INT start = bitpos_ - (bitpos_ % unit);
+      if (bitregion_start_ && start < bitregion_start_)
+	break;
+      if (bitregion_end_ && start + unit > bitregion_end_)
+	break;
+
+      *out_mode = mode_;
+      mode_ = GET_MODE_WIDER_MODE (mode_);
+      count_++;
+      return true;
+    }
+  return false;
+}
+
+/* Return true if smaller modes are generally preferred for this kind
+   of bitfield.  */
+
+bool
+bit_field_mode_iterator::prefer_smaller_modes ()
+{
+  return (volatilep_
+	  ? targetm.narrow_volatile_bitfield ()
+	  : !SLOW_BYTE_ACCESS);
+}
+
 /* Find the best machine mode to use when referencing a bit field of length
    BITSIZE bits starting at BITPOS.
 
@@ -2655,69 +2747,21 @@  get_best_mode (int bitsize, int bitpos,
 	       unsigned HOST_WIDE_INT bitregion_start,
 	       unsigned HOST_WIDE_INT bitregion_end,
 	       unsigned int align,
-	       enum machine_mode largest_mode, int volatilep)
+	       enum machine_mode largest_mode, bool volatilep)
 {
+  bit_field_mode_iterator iter (bitsize, bitpos, bitregion_start,
+				bitregion_end, align, volatilep);
+  enum machine_mode widest_mode = VOIDmode;
   enum machine_mode mode;
-  unsigned int unit = 0;
-  unsigned HOST_WIDE_INT maxbits;
-
-  /* If unset, no restriction.  */
-  if (!bitregion_end)
-    maxbits = MAX_FIXED_MODE_SIZE;
-  else
-    maxbits = bitregion_end - bitregion_start + 1;
-
-  /* Find the narrowest integer mode that contains the bit field.  */
-  for (mode = GET_CLASS_NARROWEST_MODE (MODE_INT); mode != VOIDmode;
-       mode = GET_MODE_WIDER_MODE (mode))
+  while (iter.next_mode (&mode)
+	 && (largest_mode == VOIDmode
+	     || GET_MODE_SIZE (mode) <= GET_MODE_SIZE (largest_mode)))
     {
-      unit = GET_MODE_BITSIZE (mode);
-      if (unit == GET_MODE_PRECISION (mode)
-	  && (bitpos % unit) + bitsize <= unit)
+      widest_mode = mode;
+      if (iter.prefer_smaller_modes ())
 	break;
     }
-
-  if (mode == VOIDmode
-      /* It is tempting to omit the following line
-	 if STRICT_ALIGNMENT is true.
-	 But that is incorrect, since if the bitfield uses part of 3 bytes
-	 and we use a 4-byte mode, we could get a spurious segv
-	 if the extra 4th byte is past the end of memory.
-	 (Though at least one Unix compiler ignores this problem:
-	 that on the Sequent 386 machine.  */
-      || MIN (unit, BIGGEST_ALIGNMENT) > align
-      || (largest_mode != VOIDmode && unit > GET_MODE_BITSIZE (largest_mode))
-      || unit > maxbits
-      || (bitregion_end
-	  && bitpos - (bitpos % unit) + unit > bitregion_end + 1))
-    return VOIDmode;
-
-  if ((SLOW_BYTE_ACCESS && ! volatilep)
-      || (volatilep && !targetm.narrow_volatile_bitfield ()))
-    {
-      enum machine_mode wide_mode = VOIDmode, tmode;
-
-      for (tmode = GET_CLASS_NARROWEST_MODE (MODE_INT); tmode != VOIDmode;
-	   tmode = GET_MODE_WIDER_MODE (tmode))
-	{
-	  unit = GET_MODE_BITSIZE (tmode);
-	  if (unit == GET_MODE_PRECISION (tmode)
-	      && bitpos / unit == (bitpos + bitsize - 1) / unit
-	      && unit <= BITS_PER_WORD
-	      && unit <= MIN (align, BIGGEST_ALIGNMENT)
-	      && unit <= maxbits
-	      && (largest_mode == VOIDmode
-		  || unit <= GET_MODE_BITSIZE (largest_mode))
-	      && (bitregion_end == 0
-		  || bitpos - (bitpos % unit) + unit <= bitregion_end + 1))
-	    wide_mode = tmode;
-	}
-
-      if (wide_mode != VOIDmode)
-	return wide_mode;
-    }
-
-  return mode;
+  return widest_mode;
 }
 
 /* Gets minimal and maximal values for MODE (signed or unsigned depending on