Patchwork Fix latent bug in RTL GCSE/PRE (PR57159)

login
register
mail settings
Submitter Julian Brown
Date May 3, 2013, 1:10 p.m.
Message ID <20130503141019.391ca3f4@octopus>
Download mbox | patch
Permalink /patch/241309/
State New
Headers show

Comments

Julian Brown - May 3, 2013, 1:10 p.m.
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?

Thanks,

Julian

ChangeLog

    gcc/
    * gcse.c (compute_ld_motion_mems): Invalidate non-simple mem refs
    in REG_EQUAL notes.
Steven Bosscher - May 5, 2013, 10:18 a.m.
On Fri, May 3, 2013 at 3:10 PM, Julian Brown wrote:
>     gcc/
>     * gcse.c (compute_ld_motion_mems): Invalidate non-simple mem refs
>     in REG_EQUAL notes.

Makes sense to me. Looking at REG_EQUAL notes in hash_scan_set is
something relatively new. Your patch fixes something we appear to have
overlooked.

Ciao!
Steven
Jeff Law - May 6, 2013, 3:28 p.m.
On 05/05/2013 04:18 AM, Steven Bosscher wrote:
> On Fri, May 3, 2013 at 3:10 PM, Julian Brown wrote:
>>      gcc/
>>      * gcse.c (compute_ld_motion_mems): Invalidate non-simple mem refs
>>      in REG_EQUAL notes.
>
> Makes sense to me. Looking at REG_EQUAL notes in hash_scan_set is
> something relatively new. Your patch fixes something we appear to have
> overlooked.
I'd still like to see the before/after dumps.  While I think the patch 
is reasonable as well, those dumps should make it clearer to anyone 
looking at this in the future what was going on -- particularly 
important since we don't have an in-tree port which exhibits this problem.

Jeff
Steven Bosscher - May 6, 2013, 4:27 p.m.
On Mon, May 6, 2013 at 5:28 PM, Jeff Law wrote:
> On 05/05/2013 04:18 AM, Steven Bosscher wrote:
>>
>> On Fri, May 3, 2013 at 3:10 PM, Julian Brown wrote:
>>>
>>>      gcc/
>>>      * gcse.c (compute_ld_motion_mems): Invalidate non-simple mem refs
>>>      in REG_EQUAL notes.
>>
>>
>> Makes sense to me. Looking at REG_EQUAL notes in hash_scan_set is
>> something relatively new. Your patch fixes something we appear to have
>> overlooked.
>
> I'd still like to see the before/after dumps.  While I think the patch is
> reasonable as well, those dumps should make it clearer to anyone looking at
> this in the future what was going on -- particularly important since we
> don't have an in-tree port which exhibits this problem.

The dumps are attached to the PR, and I know what is going on: Paolo
and I added support for hashing REG_EQUAL notes, to recover most (if
not all) of the PRE/HOIST opportunities lost along with libcall notes.
Before that change, the worst that could happen would have been
incorrect REG_EQUAL notes. Now that values in notes are considered as
PRE/HOIST candidates, MEMs within notes have to be invalidated. The
patch fixes something Paolo and I simply overlooked.

Ciao!
Steven
Jeff Law - May 6, 2013, 5:44 p.m.
On 05/06/2013 10:27 AM, Steven Bosscher wrote:
> On Mon, May 6, 2013 at 5:28 PM, Jeff Law wrote:
>> On 05/05/2013 04:18 AM, Steven Bosscher wrote:
>>>
>>> On Fri, May 3, 2013 at 3:10 PM, Julian Brown wrote:
>>>>
>>>>       gcc/
>>>>       * gcse.c (compute_ld_motion_mems): Invalidate non-simple mem refs
>>>>       in REG_EQUAL notes.
>>>
>>>
>>> Makes sense to me. Looking at REG_EQUAL notes in hash_scan_set is
>>> something relatively new. Your patch fixes something we appear to have
>>> overlooked.
>>
>> I'd still like to see the before/after dumps.  While I think the patch is
>> reasonable as well, those dumps should make it clearer to anyone looking at
>> this in the future what was going on -- particularly important since we
>> don't have an in-tree port which exhibits this problem.
>
> The dumps are attached to the PR,
I must have missed that notification.


  and I know what is going on: Paolo
> and I added support for hashing REG_EQUAL notes, to recover most (if
> not all) of the PRE/HOIST opportunities lost along with libcall notes.
But what's important here is that anyone be able to look at this issue 
in the future and figure out what's going on.

> Before that change, the worst that could happen would have been
> incorrect REG_EQUAL notes. Now that values in notes are considered as
> PRE/HOIST candidates, MEMs within notes have to be invalidated. The
> patch fixes something Paolo and I simply overlooked.
Understood.  My point is this needs to be clearer to folks other than 
Paolo and yourself.  For some folks, being able to look at the dumps and 
implementation side by side along with the textual explanation really helps.

As the original author of most of the RTL PRE code it certainly wasn't 
clear to me what was happening -- and that's an indication more 
information was needed.

I never did much/anything with the load/store motion bits, so I wasn't 
aware of the overall structure where we have items in the table, and 
mark them as invalid.  As far as I know load/store motion is the only 
PRE code which allows such things in the tables at all.  Now that I can 
see the dumps & follow the load/store specific bits of the 
implementation it's pretty clear now this patch is OK.

Jeff
Jeff Law - May 6, 2013, 5:53 p.m.
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?
>
> Thanks,
>
> Julian
>
> ChangeLog
>
>      gcc/
>      * gcse.c (compute_ld_motion_mems): Invalidate non-simple mem refs
>      in REG_EQUAL notes.
>
>
> pre-bugfix-1.diff
>
>
> 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.

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?

jeff
Julian Brown - May 7, 2013, 3:05 p.m.
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

Patch

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