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
>
> 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 ;))
> Anyway, here is my patch for review.
>
> OK for trunk after regression testing?
Ok.
Thanks,
Richard.
> Thanks
> Bernd.
