| Message ID | 20130503141019.391ca3f4@octopus |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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: 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
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.