Re: [patch] Fix PR middle-end/58570

[Richard Biener <richard dot guenther at gmail dot com>]

On Tue, Oct 8, 2013 at 7:52 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Probably because the actual accesses may overlap if we choose to
>> perform a bigger access.
>
> Nope, simply because they share a byte.
>
>> The same can happen if we for struct { char c1; char c2; } perform
>> an HImode access in case the target doesn't support QImode accesses.
>> Basically anytime we go through the bitfield expansion path.  Thus, doesn't
>> that mean that MEM_EXPR is wrong on the MEMs?  Maybe we used to
>> strip all DECL_BIT_FIELD component-refs at some point (adjusting
>> MEM_OFFSET accordingly)?
>
> Yes, we used to strip the MEM_EXPRs as soon as we go through the bitfield
> expansion path until last year, when I changed it:
>
> 2012-09-14  Eric Botcazou  <ebotcazou@adacore.com>
>
>         PR rtl-optimization/44194
>         * calls.c (expand_call): In the PARALLEL case, copy the return value
>         into pseudos instead of spilling it onto the stack.
>         * emit-rtl.c (adjust_address_1): Rename ADJUST into ADJUST_ADDRESS and
>         add new ADJUST_OBJECT parameter.
>         If ADJUST_OBJECT is set, drop the underlying object if it cannot be
>         proved that the adjusted memory access is still within its bounds.
>         (adjust_automodify_address_1): Adjust call to adjust_address_1.
>         (widen_memory_access): Likewise.
>         * expmed.c (store_bit_field_1): Call adjust_bitfield_address instead
>         of adjust_address.  Do not drop the underlying object of a MEM.
>         (store_fixed_bit_field): Likewise.
>         (extract_bit_field_1): Likewise.  Fix oversight in recursion.
>         (extract_fixed_bit_field): Likewise.
>         * expr.h (adjust_address_1): Adjust prototype.
>         (adjust_address): Adjust call to adjust_address_1.
>         (adjust_address_nv): Likewise.
>         (adjust_bitfield_address): New macro.
>         (adjust_bitfield_address_nv): Likewise.
>         * expr.c (expand_assignment): Handle a PARALLEL in more cases.
>         (store_expr): Likewise.
>         (store_field): Likewise.
>
> But this was done carefully, i.e. we still drop the MEM_EXPRs if we cannot
> prove that they are still valid.  Now the granularity of memory accesses at
> the RTL level is the byte so everything is rounded up to byte boundaries,
> that's why bitfields sharing a byte need to be dealt with specially.

Yes, so you need to drop bit-granular parts of MEM_EXPRs at this point
(or when initially creating them from set_mem_attributes_minus_bitpos).

>> Your patch seems to paper over this issue in the wrong place ...
>
> No, it's the proper, albeit conservative, fix in my opinion.

In my opinion the MEM_EXPR is "wrong", as it is supposed to be
the tree equivalent of the memory access.  At gimple level we
handle accesses at bit-granularity so bit-accesses are fine.
Not so at RTL level it seems.

[this also shows we probably should lower bit-granular accesses
at the gimple level, as planned for some time (to read, bit-extract
and read, bit-modify, write)]

Btw, ao_ref_from_mem will AFAIK not correctly handle bit-granular
accesses.  For

struct { int pad : 1; int a : 1; int b : 1; } x;

x.a will have MEM_SIZE == 1 and ref->offset = 1, ref->size == 8
x.b will have MEM_SIZE == 1 and ref->offset = 2, ref->size == 8

so we feed the alias oracle with bit-granular offsets but byte-granular
sizes.  Now I cannot quickly create a testcase that makes offset
based disambiguation disambugate two accesses that overlap
with the actual memory access, but I'm not 100% sure it's not
possible.  At least it will cause false aliasing for accesses crossing
byte-boundaries.

While the ultimate solution of making gimple match rtl (byte-granular
accesses only) would be best, adjusting the MEM attrs to the
RTL land sounds like a more appropriate fix.

Richard.

> --
> Eric Botcazou