Hi,
On Fri, 18 Oct 2013 14:06:57, Richard Biener wrote:
>
> On Fri, Oct 18, 2013 at 1:56 PM, Bernd Edlinger
> <bernd.edlin...@hotmail.de> wrote:
>> Hello Richard,
>>
>> I see it the same way.
>>
>> The existing code in optimize_bit_field_compare looks wrong. It is very
>> asymmetric,
>> and handles lvolatilep and rvolatilep differently. And the original handling
>> of
>> flag_strict_volatile_bitfields also was asymmetric.
>>
>> However this optimize-bit-field-compare check at the beginning of that
>> function was not
>> introduced because of volatile issues I think:
>>
>> There was a discussion in April 2012 on this thread: "Continue
>> strict-volatile-bitfields fixes"
>>
>> http://gcc.gnu.org/ml/gcc-patches/2012-04/msg01094.html
>>
I played with a sh cross-compiler and I checked the mentioned
test cases manually. Result: They will regress with this patch.
But the true reason for this is that get_inner_reference
returns a different mode for non-volatile bit-fields
dependent on the flag_strict_volatile_bitfields,
which is wrong per definition.
So I will prepare another patch that restores the
code in stor-layout.c to what it was before 2010,
and uses DECL_BIT_FIELD_TYPE in get_inner_reference
if the reference is volatile and flag_strict_volatile_bitfields.
>> The result was that this optimization seems to break other possible
>> optimizations later on,
>> when -fstrict-volatile-bitfields was enabled on the SH target. Even when the
>> bit fields are NOT volatile.
>> (Of course you should not touch volatile bit fields at all)
>>
>> And this was added to optimize_bit_field_compare as a result:
>>
>> /* In the strict volatile bitfields case, doing code changes here may prevent
>> other optimizations, in particular in a SLOW_BYTE_ACCESS setting. */
>> if (flag_strict_volatile_bitfields> 0)
>> return 0;
>
> I see it as that this check breaked the testcases which required the
> testcase fixes. So yes, if
> that referenced patch got in then it can likely be reverted.
>
>> Well, I dont know if that still applies to the trunk, and probably this was
>> the completely wrong way to fix
>> this issue it here.
>>
>> Maybe that had to do with the other place in stor-layout.c,
>> which also could be solved differently.
>>
>> I think it could be possible to remove the flag_strict_volatile_bitfields
>> special case
>> also there, and instead use the DECL_BIT_FIELD_TYPE instead of
>> DECL_BIT_FIELD in
>> get_inner_reference IF we have flag_strict_volatile_bitfields and that field
>> is really volatile. At least this helped in the portable volatiliy patch.
>> What do you think?
>
> Well, sure using DECL_BIT_FIELD is wrong if AACPS specifies the
> handling is conditional
> on being declared as bitfield. Otherwise
>
> struct {
> int c : 8;
> };
>
> will be not handled correctly (DECL_BIT_FIELD is not set, the field
> occupies a QImode).
>
> DECL_BIT_FIELD_TYPE is the proper way to check whether a field was _declared_
> as bitfield.
>
> So yes, removing this special-case in stor-layout.c seems possible -
> but beware of
> possible ABI issues here, for example passing an argument of the above type
> by value might result in passing it in a register or on the stack
> dependent on whether
> the struct has QImode or BLKmode (I'd say that's a target bug then as the
> target
> should look at the type, not at its mode... but reality is that I
> fully can believe such
> a bug exists - and whoops you have a call ABI that depends on
> -fstrict-volatile-bitfields ;))
>
I've done a grep "DECL_BIT_FIELD[^_]" and looked at all references of
this attribute. There are even places in config/i386 and config/frv where
it is used. All of these places look like latent bugs, if it depends on
-fstrict-volatile-bitfields. None of them check for volatiles.
I think restoring the original semantic of that attribute will not cause
any problems.
>> Anyway, here is my patch for review.
>>
>> OK for trunk after regression testing?
>
> Ok.
>
> Thanks,
> Richard.
>
>> Thanks
>> Bernd
