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