[patch] reimplement -fstrict-volatile-bitfields

[Sandra Loosemore]

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.

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

-Sandra