This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix latent bug in RTL GCSE/PRE (PR57159)


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
> 
> Is there any good reason why the search for the note is separated
> from the invalidation code.  As far as I can tell, both the search
> for the note and the call to invalidate_any_buried_refs ought to be
> in a single block of uninterrupted code.

No, those fragments of code could be moved together.

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

Thanks,

Julian


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]