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

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

[Julian Brown]

[-- Attachment #1: Type: text/plain, Size: 468 bytes --]

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.

[-- Attachment #2: pre-bugfix-1.diff --]
[-- Type: text/x-patch, Size: 1082 bytes --]

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

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

[Steven Bosscher]

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

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

[Jeff Law]

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

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

[Steven Bosscher]

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

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

[Jeff Law]

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

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

[Jeff Law]

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

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

[Julian Brown]

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

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

[Julian Brown]

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

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

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

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

On 05/07/2013 09:05 AM, Julian Brown wrote:
> On Mon, 6 May 2013 11:53:20 -0600
>
> 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.
For a simple mem, we'll do the right thing, at least that's my reading 
of the code.

I went ahead and put the two code fragments together and committed the 
change after a bootstrap and regression test on x86_64-unknown-linux-gnu.

For reference, attached is the patch that ultimately went into the tree.

Thanks,
Jeff





[-- Attachment #2: P --]
[-- Type: text/plain, Size: 1706 bytes --]

commit f473eb72a23bc82db0ee23e51fdd40b20417fb15
Author: law <law@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Sat May 18 03:48:18 2013 +0000

           * gcse.c (compute_ld_motion_mems): If a non-simple MEM is
           found in a REG_EQUAL note, invalidate it.
    
    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@199049 138bc75d-0d04-0410-961f-82ee72b054a4

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 4536e62..d6eab5f 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2013-05-17  Julian Brown  <julian@codesourcery.com>
+
+	* gcse.c (compute_ld_motion_mems): If a non-simple MEM is
+	found in a REG_EQUAL note, invalidate it.
+
 2013-05-17   Easwaran Raman  <eraman@google.com>
 
 	* tree-ssa-reassoc.c (find_insert_point): New function.
diff --git a/gcc/gcse.c b/gcc/gcse.c
index 07043f7..e485985 100644
--- a/gcc/gcse.c
+++ b/gcc/gcse.c
@@ -3894,6 +3894,8 @@ 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;
 
 		  /* Check for a simple LOAD...  */
 		  if (MEM_P (src) && simple_mem (src))
@@ -3910,6 +3912,15 @@ compute_ld_motion_mems (void)
 		      invalidate_any_buried_refs (src);
 		    }
 
+		  if (note != 0 && REG_NOTE_KIND (note) == REG_EQUAL)
+		    src_eq = XEXP (note, 0);
+		  else
+		    src_eq = NULL_RTX;
+
+		  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