RE: [PATCH] reimplement -fstrict-volatile-bitfields v4, part 1/2

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

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 !

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 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)))

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?

Bernd.

> Richard.
>
>> -Sandra

Attachment: patch-bitfields.diff
Description: Binary data