| Message ID | 18178507.ChivVPE93z@polaris |
|---|---|
| State | New |
| Headers | show |
On Tue, Oct 8, 2013 at 10:19 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: > Hi, > > this is a regression on the mainline introduced by my tree-ssa-alias.c change: > > 2013-04-17 Eric Botcazou <ebotcazou@adacore.com> > > * tree-ssa-alias.c (nonoverlapping_component_refs_of_decl_p): New. > (decl_refs_may_alias_p): Add REF1 and REF2 parameters. > Use nonoverlapping_component_refs_of_decl_p to disambiguate component > references. > (refs_may_alias_p_1): Adjust call to decl_refs_may_alias_p. > * tree-streamer.c (record_common_node): Adjust reference in comment. > > Unlike its model nonoverlapping_component_refs_p from alias.c, the predicate > nonoverlapping_component_refs_of_decl_p considers that different fields in the > same structure cannot overlap. While that's true in GIMPLE, that's false in > RTL for bitfields and tree-ssa-alias.c is also queried from RTL nowadays... Probably because the actual accesses may overlap if we choose to perform a bigger access. 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)? Your patch seems to paper over this issue in the wrong place ... Richard. > Therefore the attached patch just copies the missing bits from the former to > the latter. Tested on x86_64-suse-linux, OK for the mainline? > > > 2013-10-08 Eric Botcazou <ebotcazou@adacore.com> > > PR middle-end/58570 > * tree-ssa-alias.c (nonoverlapping_component_refs_of_decl_p): Return > false if both components are bitfields. > > > 2013-10-08 Eric Botcazou <ebotcazou@adacore.com> > > * gcc.c-torture/execute/pr58570.c: New test. > > > -- > Eric Botcazou
> 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. > Your patch seems to paper over this issue in the wrong place ... No, it's the proper, albeit conservative, fix in my opinion.
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
> 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. I see, but I'm nevertheless waiting for a testcase. > 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. This will disable the RTL machinery installed for PR rtl-optimization/44194, so we'll need to reopen it... Given that it seems to be working correctly, I don't see that point in doing so (and I don't intend to spend time on this right now). So let's apply my patchlet first, close PR middle-end/58570 and if you want to discuss an alternate plan for PR rtl-optimization/44194, let's do that in a separate thread.
On Wed, Oct 9, 2013 at 1:36 PM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> 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. > > I see, but I'm nevertheless waiting for a testcase. > >> 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. > > This will disable the RTL machinery installed for PR rtl-optimization/44194, > so we'll need to reopen it... Given that it seems to be working correctly, > I don't see that point in doing so (and I don't intend to spend time on this > right now). So let's apply my patchlet first, close PR middle-end/58570 and > if you want to discuss an alternate plan for PR rtl-optimization/44194, let's > do that in a separate thread. Well, ok. Please adjust the comment + /* Different fields of the same record type cannot overlap, unless they + are both bitfields and we are at the RTL level. */ to sth like ??? Bitfields can overlap at RTL level so punt if we end up at them. or sth similar. You don't check whether we are at the RTL level after all (not that this would be appropriate). Thanks, Richard. > -- > Eric Botcazou
> Well, ok. Please adjust the comment > > + /* Different fields of the same record type cannot overlap, unless > they + are both bitfields and we are at the RTL level. */ > > to sth like > > ??? Bitfields can overlap at RTL level so punt if we end up at > them. > > or sth similar. Done, thanks. > You don't check whether we are at the RTL level after all (not that this > would be appropriate). Yes, I was sure that you would have criticized that even more ;-)
Index: tree-ssa-alias.c =================================================================== --- tree-ssa-alias.c (revision 203241) +++ tree-ssa-alias.c (working copy) @@ -803,12 +803,13 @@ nonoverlapping_component_refs_of_decl_p if (type1 != type2 || TREE_CODE (type1) != RECORD_TYPE) goto may_overlap; - /* Different fields of the same record type cannot overlap. */ + /* Different fields of the same record type cannot overlap, unless they + are both bitfields and we are at the RTL level. */ if (field1 != field2) { component_refs1.release (); component_refs2.release (); - return true; + return !(DECL_BIT_FIELD (field1) && DECL_BIT_FIELD (field2)); } }