This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Fix asymmetric volatile handling in optimize_bit_field_compare


On Fri, Oct 18, 2013 at 1:56 PM, Bernd Edlinger
<bernd.edlinger@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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]