> Am 22.12.2025 um 05:34 schrieb Kugan Vivekanandarajah > <[email protected]>: > > > > > > On 19 Dec 2025, at 7:41 pm, Richard Biener <[email protected]> > > wrote: > > > > External email: Use caution opening links or attachments > > > > > > On Fri, Dec 19, 2025 at 12:52 AM Kugan Vivekanandarajah > > <[email protected]> wrote: > >> > >> Hi, > >> > >> Thanks for the review. > >> > >>> On 15 Dec 2025, at 11:56 pm, Richard Biener <[email protected]> > >>> wrote: > >>> > >>> External email: Use caution opening links or attachments > >>> > >>> > >>> On Sun, Dec 14, 2025 at 6:17 AM Kugan Vivekanandarajah > >>> <[email protected]> wrote: > >>>> > >>>> Hi, > >>>> This a patch fixes Bug 123067] by checking for partial aliasing in self > >>>> write test in LICM. > >>>> > >>>> Bootstrapped and regression tested with no new regressions. > >>>> > >>>> gcc/ChangeLog: > >>>> > >>>> 2025-12-09 Kugan Vivekanandarajah <[email protected]> > >>>> > >>>> PR middle-end/123067 > >>>> * tree-ssa-loop-im.cc (is_self_write): > >>>> > >>>> gcc/testsuite/ChangeLog: > >>>> > >>>> 2025-12-09 Kugan Vivekanandarajah <[email protected]> > >>>> > >>>> PR middle-end/123067 > >>>> * gcc.dg/licm-self-write-partial-alias.c: New test. > >>>> Is this OK? > >>> > >>> + /* Verify there is no partial aliasing. */ > >>> + if (!mem_refs_may_alias_p (load_ref, store_ref, > >>> + &memory_accesses.ttae_cache, true)) > >>> + return true; /* Disjoint: safe to hoist. */ > >>> > >>> this is redundant? If they are not aliasing then the caller would > >>> already say so? > >>> > >>> + /* They may alias. Verify exact same location. */ > >>> + return (operand_equal_p (load_ref->mem.base, store_ref->mem.base, 0) > >>> + && known_eq (load_ref->mem.size, store_ref->mem.size) > >>> + && known_eq (load_ref->mem.offset, store_ref->mem.offset)); > >>> > >>> this looks incomplete. See mem_ref_hasher::equal. > >>> That is, dependent on ->ref_decomposed the compare should look different, > >>> merging .offset with the MEM_REF offset in base. Maybe we can factor > >>> out a helper like > >>> > >>> bool im_compare_access_position_and_size (ao_ref *ref1, ao_ref *ref2) > >> > >> Tried factoring out but this is making it more complicated (due to the > >> divergence). Here is the version I tested. Please let me know > >> If you want me to post the version with > >> im_compare_access_position_and_size. > > > > I think this will now regress the case where the reference we want to > > optimize > > is variably indexed (with invariant index, of course). I'm not sure we need > > the > > > > + && load_ref->mem.volatile_p == store_ref->mem.volatile_p > > + && (load_ref->mem.ref_alias_set == store_ref->mem.ref_alias_set > > + /* We are not canonicalizing alias-sets but for the > > + special-case we didn't canonicalize yet and the > > + incoming ref is a alias-set zero MEM we pick > > + the correct one already. */ > > + || (!load_ref->ref_canonical > > + && (TREE_CODE (store_ref->mem.ref) == MEM_REF > > + || TREE_CODE (store_ref->mem.ref) == TARGET_MEM_REF) > > + && store_ref->mem.ref_alias_set == 0) > > + /* Likewise if there's a canonical ref with alias-set zero. > > */ > > + || (load_ref->ref_canonical > > + && load_ref->mem.ref_alias_set == 0))); > > > > part here, since the argument is not about TBAA. It's just the > > > > if (obj2->max_size_known_p ()) > > return (mem1->ref_decomposed > > && ((TREE_CODE (mem1->mem.base) == MEM_REF > > && TREE_CODE (obj2->base) == MEM_REF > > && operand_equal_p (TREE_OPERAND (mem1->mem.base, 0), > > TREE_OPERAND (obj2->base, 0), 0) > > && known_eq (mem_ref_offset (mem1->mem.base) * > > BITS_PER_UNIT + mem1->mem.offset, > > mem_ref_offset (obj2->base) * > > BITS_PER_UNIT + obj2->offset)) > > || (operand_equal_p (mem1->mem.base, obj2->base, 0) > > && known_eq (mem1->mem.offset, obj2->offset))) > > && known_eq (mem1->mem.size, obj2->size) > > && known_eq (mem1->mem.max_size, obj2->max_size) > > .. > > else > > return operand_equal_p (mem1->mem.ref, obj2->ref, 0); > > > > parts that are relevant. You'll have to ensure max_size_known_p agrees > > or resort to alignment considerations to rule out partial overlaps. I > > mostly > > suggested the factoring to have one place with the "delicate" handling. > > Added a TODO with the link to the message and some context. > > > > But I see this is now very complicated so I'd say go with your original > > version which should be conservatively correct, just not perfect in > > allowing all opportunities. I'll note down a TODO to try to factor this > > in a way that suits me which I guess is more effective than trying > > back-and-forth via reviews ;) > > Attached is the patch that has the TODO and old simpler implementation. > > Is this OK?
Ok Richard > Thanks, > Kugan > > > > > > Thanks, > > Richard. > > > >> Thanks, > >> Kugan > >> > >> > >> > >> > >>> > >>> for this? > >>> > >>> Thanks, > >>> Richard. > >>> > >>>> Thanks, > >>>> Kugan > >>>> > >>>> > >>>> > >>>> > >> > > <0001-Bug-123067-V3-LICM-wrong-code.patch>
