Patchwork [6/8] Add strict volatile handling to bit_field_mode_iterator

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

Comments

Richard Sandiford - Nov. 3, 2012, 11:28 a.m.
This patch makes bit_field_mode_iterator take -fstrict-volatile-bitfields
into account, in cases where the size of the underlying object is known.
This is used in the next patch.

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

Richard


gcc/
	* machmode.h (bit_field_mode_iterator::bit_field_mode_iterator):
	Add an object_bitsize parameter.
	(bit_field_mode_iterator): Add object_bitsize_.
	* stor-layout.c (bit_field_mode_iterator::bit_field_mode_iterator):
	Add an object_bitsize parameter.  Initialize object_bitsize_.
	(bit_field_mode_iterator::next_mode): For strict volatile fields,
	reject modes that aren't the same width as the underlying object.
	(get_best_mode): Update bit_field_mode_iterator constructor.
Eric Botcazou - Nov. 13, 2012, 1:56 p.m.
> This patch makes bit_field_mode_iterator take -fstrict-volatile-bitfields
> into account, in cases where the size of the underlying object is known.
> This is used in the next patch.

Do we really need to add that to the iterator?  The -fstrict-volatile-
bitfields implementation is still controversial so I'm not sure that we want
let it spread.  Can't the client code just skip the problematic modes?
Richard Sandiford - Nov. 15, 2012, 12:25 p.m.
Thanks for the reviews.

Eric Botcazou <ebotcazou@adacore.com> writes:
>> This patch makes bit_field_mode_iterator take -fstrict-volatile-bitfields
>> into account, in cases where the size of the underlying object is known.
>> This is used in the next patch.
>
> Do we really need to add that to the iterator?  The -fstrict-volatile-
> bitfields implementation is still controversial so I'm not sure that we want
> let it spread.  Can't the client code just skip the problematic modes?

The idea was to centralise the knowledge about what modes are valid
rather than requiring every client to know the rules.  From that point
of view it seems inconsistent for the new interface to handle the
bitregion_{start,end} restrictions (a correctness issue) but not the
volatility restrictions (also a correctness issue).  Especially when the
interface already knows whether the field is volatile and already handles
the choice between "narrow" and "wide" volatile bitfields.

If there's talk of removing -fstrict-volatile or changing its semantics
then handling it in the interface ought to make that easier.

Richard
Eric Botcazou - Nov. 15, 2012, 5:08 p.m.
> The idea was to centralise the knowledge about what modes are valid
> rather than requiring every client to know the rules.  From that point
> of view it seems inconsistent for the new interface to handle the
> bitregion_{start,end} restrictions (a correctness issue) but not the
> volatility restrictions (also a correctness issue).  Especially when the
> interface already knows whether the field is volatile and already handles
> the choice between "narrow" and "wide" volatile bitfields.

Richard B.'s idea is precisely to reimplement -fstrict-volatile bitfields on 
top of bitregion_{start,end}, that's why I'm not sure we want to make it part 
of the interface at all.
Richard Sandiford - Nov. 15, 2012, 5:47 p.m.
Eric Botcazou <ebotcazou@adacore.com> writes:
>> The idea was to centralise the knowledge about what modes are valid
>> rather than requiring every client to know the rules.  From that point
>> of view it seems inconsistent for the new interface to handle the
>> bitregion_{start,end} restrictions (a correctness issue) but not the
>> volatility restrictions (also a correctness issue).  Especially when the
>> interface already knows whether the field is volatile and already handles
>> the choice between "narrow" and "wide" volatile bitfields.
>
> Richard B.'s idea is precisely to reimplement -fstrict-volatile bitfields on 
> top of bitregion_{start,end}, that's why I'm not sure we want to make it part 
> of the interface at all.

OK.  The current recursive force-mem-to-reg cases in store_bit_field_1
and extract_bit_field_1 don't handle -fstrict-volatile-bitfields at all,
so this patch was trying to fix what seemed like an oversight.  Is it OK
to leave the code as-is (not handling -fstrict-volatile-bitfields),
or do I need to add new code to the expmed.c routines?

Richard
Eric Botcazou - Nov. 15, 2012, 7:30 p.m.
> OK.  The current recursive force-mem-to-reg cases in store_bit_field_1
> and extract_bit_field_1 don't handle -fstrict-volatile-bitfields at all,
> so this patch was trying to fix what seemed like an oversight.  Is it OK
> to leave the code as-is (not handling -fstrict-volatile-bitfields),
> or do I need to add new code to the expmed.c routines?

The former, I think.
Richard Sandiford - Nov. 18, 2012, 5:36 p.m.
Eric Botcazou <ebotcazou@adacore.com> writes:
>> OK.  The current recursive force-mem-to-reg cases in store_bit_field_1
>> and extract_bit_field_1 don't handle -fstrict-volatile-bitfields at all,
>> so this patch was trying to fix what seemed like an oversight.  Is it OK
>> to leave the code as-is (not handling -fstrict-volatile-bitfields),
>> or do I need to add new code to the expmed.c routines?
>
> The former, I think.

OK, I left this patch out and removed the associated constructor argument
from patch 7.

Richard

Patch

Index: gcc/machmode.h
===================================================================
--- gcc/machmode.h	2012-11-02 22:47:03.291372600 +0000
+++ gcc/machmode.h	2012-11-02 23:14:00.225368645 +0000
@@ -265,7 +265,7 @@  extern enum machine_mode mode_for_vector
 public:
   bit_field_mode_iterator (HOST_WIDE_INT, HOST_WIDE_INT,
 			   HOST_WIDE_INT, HOST_WIDE_INT,
-			   unsigned int, bool);
+			   unsigned HOST_WIDE_INT, unsigned int, bool);
   bool next_mode (enum machine_mode *);
   bool prefer_smaller_modes ();
 
@@ -277,6 +277,7 @@  extern enum machine_mode mode_for_vector
   HOST_WIDE_INT bitpos_;
   HOST_WIDE_INT bitregion_start_;
   HOST_WIDE_INT bitregion_end_;
+  unsigned HOST_WIDE_INT object_bitsize_;
   unsigned int align_;
   bool volatilep_;
   int count_;
Index: gcc/stor-layout.c
===================================================================
--- gcc/stor-layout.c	2012-11-02 23:10:07.316369215 +0000
+++ gcc/stor-layout.c	2012-11-02 23:14:00.224368645 +0000
@@ -2634,6 +2634,8 @@  fixup_unsigned_type (tree type)
    BITREGION_END - BITREGION_START + 1.  Otherwise, we are allowed to touch
    any adjacent non bit-fields.
 
+   OBJECT_BITSIZE is the number of bits that should be used to access
+   a strict volatile bitfield, or 0 if not known.
    ALIGN is the alignment of the underlying object in bits.
    VOLATILEP says whether the bitfield is volatile.  */
 
@@ -2641,11 +2643,12 @@  fixup_unsigned_type (tree type)
 ::bit_field_mode_iterator (HOST_WIDE_INT bitsize, HOST_WIDE_INT bitpos,
 			   HOST_WIDE_INT bitregion_start,
 			   HOST_WIDE_INT bitregion_end,
+			   unsigned HOST_WIDE_INT object_bitsize,
 			   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)
+  bitregion_end_ (bitregion_end), object_bitsize_ (object_bitsize),
+  align_ (MIN (align, BIGGEST_ALIGNMENT)), volatilep_ (volatilep), count_ (0)
 {
   if (bitregion_end_)
     bitregion_end_ += 1;
@@ -2676,6 +2679,14 @@  bool bit_field_mode_iterator::next_mode
       if ((bitpos_ % unit) + bitsize_ > unit)
 	continue;
 
+      /* If the field is strict volatile, skip modes that are not the same
+	 as the object size.  */
+      if (object_bitsize_
+	  && unit != object_bitsize_
+	  && volatilep_
+	  && flag_strict_volatile_bitfields > 0)
+	continue;
+
       /* Stop if the mode is too wide to handle efficiently.  */
       if (unit > MAX_FIXED_MODE_SIZE)
 	break;
@@ -2749,7 +2760,7 @@  get_best_mode (int bitsize, int bitpos,
 	       enum machine_mode largest_mode, bool volatilep)
 {
   bit_field_mode_iterator iter (bitsize, bitpos, bitregion_start,
-				bitregion_end, align, volatilep);
+				bitregion_end, 0, align, volatilep);
   enum machine_mode widest_mode = VOIDmode;
   enum machine_mode mode;
   while (iter.next_mode (&mode)