* [PATCH] Fix latent bug in RTL GCSE/PRE (PR57159) @ 2013-05-03 13:10 Julian Brown 2013-05-05 10:19 ` Steven Bosscher 2013-05-06 17:53 ` Jeff Law 0 siblings, 2 replies; 9+ messages in thread From: Julian Brown @ 2013-05-03 13:10 UTC (permalink / raw) To: gcc-patches; +Cc: Steven Bosscher [-- 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix latent bug in RTL GCSE/PRE (PR57159) 2013-05-03 13:10 [PATCH] Fix latent bug in RTL GCSE/PRE (PR57159) Julian Brown @ 2013-05-05 10:19 ` Steven Bosscher 2013-05-06 15:28 ` Jeff Law 2013-05-06 17:53 ` Jeff Law 1 sibling, 1 reply; 9+ messages in thread From: Steven Bosscher @ 2013-05-05 10:19 UTC (permalink / raw) To: Julian Brown; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix latent bug in RTL GCSE/PRE (PR57159) 2013-05-05 10:19 ` Steven Bosscher @ 2013-05-06 15:28 ` Jeff Law 2013-05-06 16:28 ` Steven Bosscher 0 siblings, 1 reply; 9+ messages in thread From: Jeff Law @ 2013-05-06 15:28 UTC (permalink / raw) To: Steven Bosscher; +Cc: Julian Brown, gcc-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix latent bug in RTL GCSE/PRE (PR57159) 2013-05-06 15:28 ` Jeff Law @ 2013-05-06 16:28 ` Steven Bosscher 2013-05-06 17:45 ` Jeff Law 0 siblings, 1 reply; 9+ messages in thread From: Steven Bosscher @ 2013-05-06 16:28 UTC (permalink / raw) To: Jeff Law; +Cc: Julian Brown, gcc-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix latent bug in RTL GCSE/PRE (PR57159) 2013-05-06 16:28 ` Steven Bosscher @ 2013-05-06 17:45 ` Jeff Law 0 siblings, 0 replies; 9+ messages in thread From: Jeff Law @ 2013-05-06 17:45 UTC (permalink / raw) To: Steven Bosscher; +Cc: Julian Brown, gcc-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix latent bug in RTL GCSE/PRE (PR57159) 2013-05-03 13:10 [PATCH] Fix latent bug in RTL GCSE/PRE (PR57159) Julian Brown 2013-05-05 10:19 ` Steven Bosscher @ 2013-05-06 17:53 ` Jeff Law 2013-05-07 15:06 ` Julian Brown 1 sibling, 1 reply; 9+ messages in thread From: Jeff Law @ 2013-05-06 17:53 UTC (permalink / raw) To: Julian Brown; +Cc: gcc-patches, Steven Bosscher 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix latent bug in RTL GCSE/PRE (PR57159) 2013-05-06 17:53 ` Jeff Law @ 2013-05-07 15:06 ` Julian Brown 2013-05-15 13:29 ` Julian Brown 2013-05-18 3:53 ` Jeff Law 0 siblings, 2 replies; 9+ messages in thread From: Julian Brown @ 2013-05-07 15:06 UTC (permalink / raw) To: Jeff Law; +Cc: gcc-patches, Steven Bosscher 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix latent bug in RTL GCSE/PRE (PR57159) 2013-05-07 15:06 ` Julian Brown @ 2013-05-15 13:29 ` Julian Brown 2013-05-18 3:53 ` Jeff Law 1 sibling, 0 replies; 9+ messages in thread From: Julian Brown @ 2013-05-15 13:29 UTC (permalink / raw) To: Julian Brown; +Cc: Jeff Law, gcc-patches, Steven Bosscher 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix latent bug in RTL GCSE/PRE (PR57159) 2013-05-07 15:06 ` Julian Brown 2013-05-15 13:29 ` Julian Brown @ 2013-05-18 3:53 ` Jeff Law 1 sibling, 0 replies; 9+ messages in thread From: Jeff Law @ 2013-05-18 3:53 UTC (permalink / raw) To: Julian Brown; +Cc: gcc-patches, Steven Bosscher [-- 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-05-18 3:53 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-05-03 13:10 [PATCH] Fix latent bug in RTL GCSE/PRE (PR57159) Julian Brown 2013-05-05 10:19 ` Steven Bosscher 2013-05-06 15:28 ` Jeff Law 2013-05-06 16:28 ` Steven Bosscher 2013-05-06 17:45 ` Jeff Law 2013-05-06 17:53 ` Jeff Law 2013-05-07 15:06 ` Julian Brown 2013-05-15 13:29 ` Julian Brown 2013-05-18 3:53 ` Jeff Law
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).