On Tue, 7 May 2013 16:05:50 +0100 Julian Brown <jul...@codesourcery.com> wrote:
> On Mon, 6 May 2013 11:53:20 -0600 > Jeff Law <l...@redhat.com> wrote: > > > On 05/03/2013 07:10 AM, Julian Brown wrote: > > > Hi, > > > > > > This is a patch which fixes a latent bug in RTL GCSE/PRE, > > > described more fully in: > > > > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57159 > > > > > > I haven't been able to reproduce the problem on mainline (nor on a > > > supported target). Maybe someone more familiar with the code in > > > question than I am can tell if the patch is correct nonetheless? > > > > Index: gcc/gcse.c > > > =================================================================== > > > --- gcc/gcse.c (revision 198175) > > > +++ gcc/gcse.c (working copy) > > > @@ -3888,6 +3888,13 @@ compute_ld_motion_mems (void) > > > { > > > rtx src = SET_SRC (PATTERN (insn)); > > > rtx dest = SET_DEST (PATTERN (insn)); > > > + rtx note = find_reg_equal_equiv_note (insn); > > > + rtx src_eq; > > > + > > > + if (note != 0 && REG_NOTE_KIND (note) == > > > REG_EQUAL) > > > + src_eq = XEXP (note, 0); > > > + else > > > + src_eq = NULL_RTX; > > > > > > /* Check for a simple LOAD... */ > > > if (MEM_P (src) && simple_mem (src)) > > > @@ -3904,6 +3911,12 @@ compute_ld_motion_mems (void) > > > invalidate_any_buried_refs (src); > > > } > > > > > > + /* Also invalidate any buried loads which may > > > be present in > > > + REG_EQUAL notes. */ > > > + if (src_eq != NULL_RTX > > > + && !(MEM_P (src_eq) && simple_mem > > > (src_eq))) > > > + invalidate_any_buried_refs (src_eq); > > > + > > > /* Check for stores. Don't worry about aliased > > > ones, they will block any movement we might do later. We only care > > > about this exact pattern since those are > > > the only > > What happens if the code contains a simple mem? We don't invalidate > > it as far as I can tell. Doesn't that open us up to the same > > problems that we're seeing with with the non-simple mem? > > I don't know. My assumption was that a "simple" mem would be one that > the PRE pass could handle -- that clause was intended to handle simple > mems in REG_EQUAL notes the same as simple mems as the source of a > set. Maybe that is not safe though, and it would be better to > unconditionally invalidate buried mems in REG_EQUAL notes? I was > slightly wary of inhibiting genuine optimisation opportunities. A not-very-scientific data point concerning the last part: I tried a patch which invalidated buried refs in any REG_EQUAL note, i.e.: --- gcc/gcse.c (revision 198880) +++ gcc/gcse.c (working copy) @@ -3894,6 +3894,7 @@ compute_ld_motion_mems (void) { rtx src = SET_SRC (PATTERN (insn)); rtx dest = SET_DEST (PATTERN (insn)); + rtx note = find_reg_equal_equiv_note (insn); /* Check for a simple LOAD... */ if (MEM_P (src) && simple_mem (src)) @@ -3910,6 +3911,11 @@ compute_ld_motion_mems (void) invalidate_any_buried_refs (src); } + /* Also invalidate any buried loads which may be present in + REG_EQUAL notes. */ + if (note != NULL_RTX && REG_NOTE_KIND (note) == REG_EQUAL) + invalidate_any_buried_refs (XEXP (note, 0)); + /* Check for stores. Don't worry about aliased ones, they will block any movement we might do later. We only care about this exact pattern since those are the only Running a bootstrap for x86_64, this gives a cc1 binary: -rwxr-xr-x 1 jbrown eeg 123362809 2013-05-15 03:24 cc1 Without the patch, the binary is slightly smaller: -rwxr-xr-x 1 jbrown eeg 123362481 2013-05-15 02:45 cc1 With the previously-posted patch which does not invalidate buried refs for simple mems, I get: -rwxr-xr-x 1 jbrown eeg 123362377 2013-05-15 04:08 cc1 i.e. slightly *smaller* than the baseline. I haven't investigated more deeply than that, but that seems to be a tiny bit of evidence at least that unconditionally invalidating buried refs (as above) might not be a good idea. There are no testsuite regressions with the above patch (for gcc/g++/libstdc++), FWIW. Cheers, Julian
