This is the mail archive of the
mailing list for the GCC project.
RE: [PATCH] reimplement -fstrict-volatile-bitfields v4, part 1/2
- From: Bernd Edlinger <bernd dot edlinger at hotmail dot de>
- To: Sandra Loosemore <sandra at codesourcery dot com>
- Cc: Richard Biener <richard dot guenther at gmail dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 29 Oct 2013 09:51:34 +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>
On Mon, 28 Oct 2013 21:29:24, Sandra Loosemore wrote:
> On 10/28/2013 03:20 AM, Bernd Edlinger wrote:
>> On Sun, 20 Oct 2013 20:23:49, Sandra Loosemore wrote:
>>> I tried a backport to GCC 4.8 and tested on arm-none-eabi. On the new
>>> pr56997-1.c testcase, it got stuck in infinite recursion between
>>> store_split_bit_field/store_fixed_bit_field and/or
>>> extract_split_bit_field/extract_fixed_bit_field. This didn't show up in
>>> my previous mainline testing.
>> 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.
> Thanks for picking up the ball on this -- I've been busy with other
> tasks for several days.
> I again tried backporting the patch series along with your fix to GCC
> 4.8 and tested on arm-none-eabi. I found that it was still getting
> stuck in infinite recursion unless the test from this patch hunk
Actually I used your path on a clean 4.8.2 and built for --target=arm-eabi.
I have seen the recursion on the extract_bit_field, but not on store_bit_field
so far, maybe you could give me a hint which test case exposes the other
flavour of this recursion problem.
>> @@ -1699,6 +1711,14 @@ extract_bit_field (rtx str_rtx, unsigned
>> enum machine_mode mode1;
>> + /* Handle extraction of unaligned fields,
>> + this can happen in -fstrict-volatile-bitfields. */
>> + if (GET_MODE_BITSIZE (GET_MODE (str_rtx)) != 0
>> + && GET_MODE_BITSIZE (GET_MODE (str_rtx)) < GET_MODE_BITSIZE (word_mode)
>> + && bitnum % GET_MODE_BITSIZE (GET_MODE (str_rtx)) + bitsize
>> +> GET_MODE_BITSIZE (GET_MODE (str_rtx)) )
>> + str_rtx = adjust_address (str_rtx, word_mode, 0);
>> /* Handle -fstrict-volatile-bitfields in the cases where it applies. */
>> if (GET_MODE_BITSIZE (GET_MODE (str_rtx))> 0)
>> mode1 = GET_MODE (str_rtx);
> was also inserted into store_bit_field.
> I also think that, minimally, this needs to be rewritten as something like
> if (MEM_P (str_rtx)
> && STRICT_ALIGNMENT
> && GET_MODE_BITSIZE (GET_MODE (str_rtx)) != 0
> && GET_MODE_BITSIZE (GET_MODE (str_rtx)) < GET_MODE_BITSIZE
> && (bitnum % MEM_ALIGN (str_rtx) + bitsize
>> GET_MODE_BITSIZE (GET_MODE (str_rtx))))
> str_rtx = adjust_address (str_rtx, word_mode, 0);
Yes, that looks fine.
> Otherwise, we'll end up using word_mode instead of the field mode on
> targets that support unaligned accesses. I tested that (again, 4.8 and
> arm-none-eabi) and results looked good, but of course ARM isn't one of
> the targets that supports unaligned accesses. :-S
> I'm still not convinced this is the right fix, though. It seems to me
> that callers of store_bit_field and extract_bit_field in expr.c ought
> not to be messing with the mode of the rtx when
> flag_strict_volatile_bitfields> 0, because that is losing information
> about the original mode that we are trying to patch up here by forcing
> it to word_mode. Instead, I think the callers ought to be passing the
> declared field mode into store_bit_field and extract_bit_field, and
> those functions ought to change the mode of the incoming rtx to match
> the field mode only if strict_volatile_bitfield_p assures us that the
> insertion/extraction can be done in that mode.
The problem starts likely at expr.c when this is done:
if (volatilep && flag_strict_volatile_bitfields> 0)
to_rtx = adjust_address (to_rtx, mode1, 0);
this restricts the possible access mode not only for bit-fields
but for all possible volatile members. But -fstrict-volatile-bitfields
is supposed to affect bit-fields only.
> 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.
> Either way, still needs more work, and more thorough testing (more
> targets, and obviously trunk as well as the 4.8 backport) before I'd
> consider this ready to commit. I might or might not be able to find
> some more time to work on this in the next week....
And it would be good to reach Richard's Nov-21 deadline for GCC-4.9