This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] reimplement -fstrict-volatile-bitfields v4, part 1/2
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Bernd Edlinger <bernd dot edlinger at hotmail dot de>
- Cc: Sandra Loosemore <sandra at codesourcery dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 14 Nov 2013 16:31:10 +0100
- Subject: Re: [PATCH] reimplement -fstrict-volatile-bitfields v4, part 1/2
- Authentication-results: sourceware.org; auth=none
- References: <52463CA4 dot 7060303 at codesourcery dot com> <CAFiYyc1z=BVSME0317W7HVr9HAvZ0PvKsdT9Yx-vsmNgEjE52g at mail dot gmail dot com> <526161EC dot 5070200 at codesourcery dot com> <325c359b-22e1-43dc-8050-5a11deb66e95 at email dot android dot com> <52649035 dot 6000802 at codesourcery dot com> <DUB122-W9C01556B8690D446546A6E4080 at phx dot gbl> <526F2B94 dot 8020907 at codesourcery dot com> <DUB122-W1396BF39760E97A090742BE4090 at phx dot gbl> <5271A851 dot 6000808 at codesourcery dot com> <CAFiYyc3bYPF5xSXSSq-3ZU-dT5Ou00x=qC-u5sXxkZVf3tH86g at mail dot gmail dot com> <DUB122-W25E69525F3AA97742A1188E4F80 at phx dot gbl>
On Thu, Nov 14, 2013 at 11:16 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi,
>
> sorry, for the delay.
> Sandra seems to be even more busy than me...
>
> Attached is a combined patch of the original part 1, and the update,
> in diff -up format.
>
> On Mon, 11 Nov 2013 13:10:45, Richard Biener wrote:
>>
>> On Thu, Oct 31, 2013 at 1:46 AM, Sandra Loosemore
>> <sandra@codesourcery.com> wrote:
>>> On 10/29/2013 02:51 AM, Bernd Edlinger wrote:
>>>>
>>>>
>>>> On Mon, 28 Oct 2013 21:29:24, Sandra Loosemore wrote:
>>>>>
>>>>> On 10/28/2013 03:20 AM, Bernd Edlinger wrote:
>>>>>>
>>>>>> I have attached an update to your patch, that should
>>>>>> a) fix the recursion problem.
>>>>>> b) restrict the -fstrict-volatile-bitfields to not violate the C++
>>>>>> memory model.
>>>
>>>
>>> Here's a new version of the update patch.
>>>
>>>
>>>>> Alternatively, if strict_volatile_bitfield_p returns false but
>>>>> flag_strict_volatile_bitfields> 0, then always force to word_mode and
>>>>> change the -fstrict-volatile-bitfields documentation to indicate that's
>>>>> the fallback if the insertion/extraction cannot be done in the declared
>>>>> mode, rather than claiming that it tries to do the same thing as if
>>>>> -fstrict-volatile-bitfields were not enabled at all.
>>>
>>>
>>> I decided that this approach was more expedient, after all.
>>>
>>> I've tested this patch (in conjunction with my already-approved but
>>> not-yet-applied patch) on mainline for arm-none-eabi, x86_64-linux-gnu, and
>>> mips-linux gnu. I also backported the entire series to GCC 4.8 and tested
>>> there on arm-none-eabi and x86_64-linux-gnu. OK to apply?
>>
>> Hm, I can't seem to find the context for
>>
>> @@ -923,6 +935,14 @@
>> store_fixed_bit_field (str_rtx, bitsize, bitnum, 0, 0, value);
>> return;
>> }
>> + else if (MEM_P (str_rtx)
>> + && MEM_VOLATILE_P (str_rtx)
>> + && flag_strict_volatile_bitfields> 0)
>> + /* This is a case where -fstrict-volatile-bitfields doesn't apply
>> + because we can't do a single access in the declared mode of the field.
>> + Since the incoming STR_RTX has already been adjusted to that mode,
>> + fall back to word mode for subsequent logic. */
>> + str_rtx = adjust_address (str_rtx, word_mode, 0);
>>
>> /* Under the C++0x memory model, we must not touch bits outside the
>> bit region. Adjust the address to start at the beginning of the
>>
>> and the other similar hunk. I suppose they apply to earlier patches
>> in the series? I suppose the above applies to store_bit_field (diff -p
>> really helps!). Why would using word_mode be any good as
>> fallback? That is, why is "Since the incoming STR_RTX has already
>> been adjusted to that mode" not the thing to fix?
>>
>
> Well, this hunk does not force the access to be in word_mode.
>
> Instead it allows get_best_mode to choose the access to be in any mode from
> QI to word_mode.
>
> It is there to revert the effect of this weird code in expr.c (expand_assigment):
>
> if (volatilep && flag_strict_volatile_bitfields> 0)
> to_rtx = adjust_address (to_rtx, mode1, 0);
>
> Note that this does not even check if the access is on a bit-field !
Then why not remove that ...
> The problem with the strict_volatile_bitfields is that it is used already
> before the code reaches store_bit_field or extract_bit_field.
>
> It starts in get_inner_reference, (which is not only used in expand_assignment
> and expand_expr_real_1)
>
> Then this,
>
> if (volatilep && flag_strict_volatile_bitfields> 0)
> op0 = adjust_address (op0, mode1, 0);
... and this ...
> and then this,
>
> /* If the field is volatile, we always want an aligned
> access. Do this in following two situations:
> 1. the access is not already naturally
> aligned, otherwise "normal" (non-bitfield) volatile fields
> become non-addressable.
> 2. the bitsize is narrower than the access size. Need
> to extract bitfields from the access. */
> || (volatilep && flag_strict_volatile_bitfields> 0
> && (bitpos % GET_MODE_ALIGNMENT (mode) != 0
> || (mode1 != BLKmode
> && bitsize < GET_MODE_SIZE (mode1) * BITS_PER_UNIT)))
... or this ...
> As a result, a read access to an unaligned volatile data member does
> not even reach the expand_bit_field if flag_strict_volatile_bitfields <= 0,
> and instead goes through convert_move (target, op0, unsignedp).
>
> I still believe the proposed patch is guaranteed to not change anything if
> -fno-strict-volatile-bitfields is used, and even if we can not guarantee
> that it creates exactly the same code for cases where the strict-volatile-bitfields
> does not apply, it certainly generates valid code, where we had invalid code,
> or ICEs without the patch.
>
> OK for trunk?
Again, most of the patch is ok (and nice), the
store_bit_field/extract_bit_field changes
point to the above issues which we should rather fix than trying
to revert them after the fact.
Why is that not possible?
That said,
+ else if (MEM_P (str_rtx)
+ && MEM_VOLATILE_P (str_rtx)
+ && flag_strict_volatile_bitfields > 0)
+ /* This is a case where -fstrict-volatile-bitfields doesn't apply
+ because we can't do a single access in the declared mode of the field.
+ Since the incoming STR_RTX has already been adjusted to that mode,
+ fall back to word mode for subsequent logic. */
+ str_rtx = adjust_address (str_rtx, word_mode, 0);
we are looking at an access with bitsize / bitregion properties so plainly
choosing word_mode here looks wrong to me. Yes, only
-fstrict-volatile-bitfields is affected but still ...
The patch is ok if you look at the above as followup.
Thanks,
Richard.
> Bernd.
>
>> Richard.
>>
>>> -Sandra