On Tue, Oct 8, 2013 at 7:52 PM, Eric Botcazou <ebotca...@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 <ebotca...@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
