On Thu, May 29, 2025 at 7:42 AM Andrew Pinski <pins...@gmail.com> wrote:
>
> On Mon, May 26, 2025 at 1:40 PM Andrew Pinski <pins...@gmail.com> wrote:
> > > Note this is redundant store removal - I'm not sure operand_equal_p
> > > is good enough to catch all cases of effective type changes done?
> > > Esp. as infering the old effective type from the read side (src2)
> > > is impossible in principle. Consider
> > >
> > > (T{ref-all} *)dest = {};
> > > dest2 = dest;
> > > dest = dest2;
> > >
> > > now, the last stmt is good to elide (we'll keep a ref-all store), but can
> > > we construct a case where it goes wrong? Possibly when 'dest'
> > > is a may-alias type but the first store (not visible in the IL as far
> > > as you analyze) was done with the ref-all stripped?
>
> Yes I finally was able to get a case where removing will fail now.
> Attached a testcase.
> So I will remove the code which deletes the store to itself.
> I will also add the testcase too.
> Also it looks like DSE needs to be fixed too:
> Deleted dead store: # .MEM_9 = VDEF <.MEM_8>
> MEM <unsigned char[128]> [(char * {ref-all})&a] = MEM <unsigned
> char[128]> [(char * {ref-all})&a];
> Or maybe we should only delete them if not ref-all?
Note ref-all isn't really the speciality - it's just the easiest case to trigger
the issue. The more complicated cases appear with unions (didn't try
unions of aggregates, see the PR below).
> I noticed that we also remove all `memcpy(a,a,N);` and
> `memmove(a,a,N);` way beforehand too. Are those supposed to be reset
> alias barriers?
> because if you replace the two memcpy in h with `memmove(a, a,
> sizeof(a));` you run into the same issue.
>
> I am getting a suspicion we need a different way to mark this rather
> than keeping around a full on memcpy.
> Is there a clean way of representing this?
Not today. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101641
where I wrote about some possibilities.
> What about doing:
> MEM <unsigned char[0]> [(char * {ref-all})&a] = {CLOBBER}
> Or something to that effect?
But that has semantics, namely make the contents of the memory
undefined. The only thing with defined semantics that we could
use would be
MEM <unsigned char[128]> [(new-alias-set-T *)&a] = MEM <unsigned
char[128]> [(old-alias-set-T *)&a];
but that generates code in the end, and since the issue prevails on RTL
we have to have an insn for this (but possibly could do with non-recognized).
So we'd have to preserve this to the end but not generate any code (and consider
it having zero size, basically like a debug-insn).
>
> Do I/we need to solve this before this patch can go in or is it enough
> to workaround the issue in this part and solve the other parts of GCC
> later on?
It's not required to solve this. I was just wary of adding another case of
this transform we know is problematic in other places.
Thanks,
Richard.
>
> Thanks,
> Andrew
