This is the mail archive of the
mailing list for the GCC project.
Re: Fix asymmetric volatile handling in optimize_bit_field_compare
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Bernd Edlinger <bernd dot edlinger at hotmail dot de>
- Cc: "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Date: Fri, 18 Oct 2013 14:06:57 +0200
- Subject: Re: Fix asymmetric volatile handling in optimize_bit_field_compare
- Authentication-results: sourceware.org; auth=none
- References: <DUB122-W224190DA915203ED7F0601E4060 at phx dot gbl>
On Fri, Oct 18, 2013 at 1:56 PM, Bernd Edlinger
> 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"
> 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
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_
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
> Anyway, here is my patch for review.
> OK for trunk after regression testing?