> 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? Thanks, Kugan > > Thanks, > Richard. > >> Thanks, >> Kugan >> >> >> >> >>> >>> for this? >>> >>> Thanks, >>> Richard. >>> >>>> Thanks, >>>> Kugan >>>> >>>> >>>> >>>> >>
0001-Bug-123067-V3-LICM-wrong-code.patch
Description: 0001-Bug-123067-V3-LICM-wrong-code.patch
