Re: Possible bug in cse.c affecting pre/post-modify mem access

[Richard Sandiford <richard dot sandiford at linaro dot org>]

Hi,

"A. Skrobov" <tyomitch@gmail.com> writes:
> Hello,
>
> While working on a port of GCC for a non-public architecture that has
> pre/post-modify memory access instructions, I discovered what looks
> like a bug which can cause incorrect code generation.
>
> My suggested fix is trivial:
> https://github.com/tyomitch/gcc/commit/7d9cc102adf11065358d4694109ce3e9f0b5c642
> -- but I cannot submit this patch without a testcase, and my expertise
> in the standard GCC target architectures is insufficient for
> reproducing this bug in any one of them. So, perhaps a maintainer of
> any supported architecture having pre/post-modify memory access can
> take a look at this?
>
> Basically, it seems to me that if a BB has a sequence like (using C
> syntax for clarity)
>
> r1 = r2 + 42;
> r3 = *r1++;
> r4 = *(r2 + 42);
>
> --then the cse pass overlooks the modification of r1 by the second
> instruction, and changes the last instruction to "r4 = *r1"

Yeah, I can't see off-hand how this would be handled correctly
by current sources.  I think the issue's probably latent on
in-tree targets since cse runs before inc_dec.

I don't think the hash function itself is the right place to
invalidate the cache though.  Any instruction with a pre/post
modification needs to have a REG_INC note as well, so iterating
over the REG_INC notes in invalidate_from_sets_and_clobbers
should be enough.  Does the attached (completely untested :-))
patch work for your test case?

A more elaborate fix would be to model the inc or dec as a dummy
set in find_sets_in_insn, so that we can still CSE the register
after it has been modified, but that would be hard to test
with the current pass order.

Thanks,
Richard


Index: gcc/cse.c
===================================================================
--- gcc/cse.c	2018-05-10 21:29:33.320961107 +0100
+++ gcc/cse.c	2018-05-10 21:29:33.399958000 +0100
@@ -6195,6 +6195,9 @@ invalidate_from_sets_and_clobbers (rtx_i
 	    invalidate (SET_DEST (y), VOIDmode);
 	}
     }
+
+  for (rtx note = REG_NOTES (insn); note; note = XEXP (note, 1))
+    invalidate (XEXP (note, 0), VOIDmode);
 }
 
 /* Process X, part of the REG_NOTES of an insn.  Look at any REG_EQUAL notes