On Fri, Jun 14, 2013 at 6:31 PM, Sandra Loosemore
<san...@codesourcery.com> wrote:
> On 06/14/2013 06:31 AM, Richard Biener wrote:
>
>> I think we can split the patch up, so let me do piecewise approval of
>> changes.
>>
>> The changes that remove the packedp flag passing around and remove
>> the warning code are ok. The store_bit_field_1 change is ok, as is
>> the similar extract_bit_field_1 change (guard the other register copying
>> path).
>>
>> Please split those out and commit them separately (I suppose they are
>> strict progressions in testing).
>
>
> I will work on splitting the patch, but I feel a little uncomfortable about
> starting to commit it piecewise without some more clear indication that this
> is the right direction. Otherwise we'll just be back in the situation where
> things are still inconsistent and broken, but in a different way than
> before.
>
>
>> That leaves a much smaller set of changes for which I have one comment
>> for now:
>>
>> @@ -4508,6 +4508,16 @@ get_bit_range (unsigned HOST_WIDE_INT *b
>>
>> gcc_assert (TREE_CODE (exp) == COMPONENT_REF);
>>
>> + /* If -fstrict-volatile-bitfields was given and this is a volatile
>> + access, then we must ignore any DECL_BIT_FIELD_REPRESENTATIVE and
>> + do the access in the mode of the field. */
>> + if (TREE_THIS_VOLATILE (exp)
>> + && flag_strict_volatile_bitfields > 0)
>> + {
>> + *bitstart = *bitend = 0;
>> + return;
>> + }
>>
>> with the past reviews where I said the implementation was broken anyway
>> I was hinting at that DECL_BIT_FIELD_REPRESENTATIVE should simply
>> be chosen in a way to adhere to flag_strict_volatile_bitfields. I had the
>> impression that this alone should be enough to implement strict volatile
>> bitfields correctly and no code in the backend would need to check
>> for flag_strict_volatile_bitfields. I may of course be wrong here, as
>> -fstrict-volatile-bitfields may apply to cases where the bitfield is _not_
>> declared volatile but only the decl? Thus,
>>
>> struct X {
>> int i : 3;
>> };
>>
>> volatile struct X x;
>>
>> Is x.i an access that needs to adhere to strict volatile bitfield
>> accesses?
>
>
> Yes, exactly; see the new pr23623.c test case included with the patch, for
> instance. Or the volatile bitfield access could be through a
> volatile-qualified pointer, as in the volatile-bitfields-3.c test case.
> Bernd Schmidt and I had some internal discussion about this and neither of
> us saw how messing with DECL_BIT_FIELD_REPRESENTATIVE could help where the
> field is not declared volatile but the object being referenced is.
Yeah, I can see that.
>> Note that IIRC whether you short-cut get_bit_range in the above way
>> you still get the access to use the mode of the FIELD_DECL.
>
>
> As I noted in my previous message, the pr23623.c test case was failing on
> all the targets I tried without this patch hunk, so the access was clearly
> not using the mode of the FIELD_DECL without it.
>
>
>> If the above constitutes a strict volatile bitfield access then the very
>> correct implementation of strict volatile bitfield accesses is to
>> adjust the memory accesses the frontends generate (consider
>> LTO and a TU compiled with -fstrict-volatile-bitfields and another TU
>> compiled with -fno-strict-volatile-bitfields). Which it probably does
>> reasonably well already (setting TREE_THIS_VOLATILE on the
>> outermost bit-field COMPONENT_REF). If that is not preserved
>> then converting the accesses to BIT_FIELD_REFs is another
>> possibility. That said, the behavior of GIMPLE IL should not change
>> dependent on a compile flag.
>
>
> I am kind of lost, here. -fstrict-volatile-bitfields is a code generation
> option, not a front end option, and it seems like LTO should not need any
> special handling here any more than it does for any of the gazillion other
> code generation options supported by GCC.
LTO doesn't have any handling of code-generation options. And it cannot
reliably. It has a few hacks to support mixed -fstrict-aliasing units for
example, but it will be completely lost if you mix -fstrict-volatile-bitfields
with -fno-strict-volatile-bitfields TUs.
Of course the easiest solution here is always adhere to the ABI and
simply drop the command-line flag. Make it a target hook instead.
That said, with the goal to have bitfield accesses lowered to their
final memory access patterns way earlier than RTL expansion we'll end
up using BIT_FIELD_REF-like accesses for the strict volatile bitfields
at some point anyway.
Richard.
> -Sandra
>
