diff mbox

Fix latent bug in RTL GCSE/PRE (PR57159)

Message ID 20130515142929.2c54729b@octopus
State New
Headers show

Commit Message

Julian Brown May 15, 2013, 1:29 p.m. UTC 
On Tue, 7 May 2013 16:05:50 +0100
Julian Brown <julian@codesourcery.com> wrote:

> On Mon, 6 May 2013 11:53:20 -0600
> Jeff Law <law@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.:


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
diff mbox

Patch

--- 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