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
