| Message ID | 87txt7dix7.fsf@talisman.home |
|---|---|
| State | New |
| Headers | show |
> 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?
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
> 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.
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
> 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.
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
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)