| Message ID | 512772F5.3040007@ispras.ru |
|---|---|
| State | New |
| Headers | show |
On 02/22/13 06:30, Andrey Belevantsev wrote: > Hello, > > As found by Jakub and explained in the PR audit trail by Alexander, this > patch fixes the selective scheduler merge glitch of 2008 that added the > unnecessary JUMP_P check to the flush_pending_lists call. I have > removed the check and expanded the binary negation for clarity. > > The patch was tested on x86-64, ia64, and ppc64 to be safe. The patch > should be conservatively safe at this stage as it adds more flushes and > thus more dependencies to the scheduler. The original test is fixed, > but I don't know how to add the test checking assembly insns order to > the testsuite. > > OK for trunk? > > Andrey > > 2012-02-22 Alexander Monakov <amonakov@ispras.ru> > Andrey Belevantsev <abel@ispras.ru> > > PR middle-end/56077 > * sched-deps.c (sched_analyze_insn): When reg_pending_barrier, > flush pending lists also on non-jumps. Can you explain how avoiding pushing the pending lists in this situation avoids coalescing the volatile stores? Also, if we go forward with your patch, the comment related to this conditional needs to be fixed -- it still says "Flush pending lists on jumps, ...", but you've removed the jump check. Jeff
On Fri, 22 Feb 2013, Jeff Law wrote: > On 02/22/13 06:30, Andrey Belevantsev wrote: > > Hello, > > > > As found by Jakub and explained in the PR audit trail by Alexander, this > > patch fixes the selective scheduler merge glitch of 2008 that added the > > unnecessary JUMP_P check to the flush_pending_lists call. I have > > removed the check and expanded the binary negation for clarity. > Can you explain how avoiding pushing the pending lists in this situation > avoids coalescing the volatile stores? You must be referring to the PR audit trail, right? I'm sure the bug reporter is mistaken that the stores are coalesced. What happens is that two of the three stores are moved up above the first asm, but because of how the awk script cuts the generated code, that is not observed. > Also, if we go forward with your patch, the comment related to this > conditional needs to be fixed -- it still says "Flush pending lists on jumps, > ...", but you've removed the jump check. Yeah, I'd say the comment is confusing both before and after the patch. Perhaps something like "Don't flush pending lists on speculation checks during selective scheduling" would be better. Alexander
On 02/22/13 07:16, Alexander Monakov wrote: > > > You must be referring to the PR audit trail, right? I'm sure the bug reporter > is mistaken that the stores are coalesced. What happens is that two of the > three stores are moved up above the first asm, but because of how the awk > script cuts the generated code, that is not observed. Thanks. That makes a lot more sense. >> Also, if we go forward with your patch, the comment related to this >> conditional needs to be fixed -- it still says "Flush pending lists on jumps, >> ...", but you've removed the jump check. > > Yeah, I'd say the comment is confusing both before and after the patch. > Perhaps something like "Don't flush pending lists on speculation checks during > selective scheduling" would be better. Sounds good. Approved with that comment change. Thanks, jeff
On 2013-02-22 18:21, Jeff Law wrote: > On 02/22/13 07:16, Alexander Monakov wrote: >> >> >> You must be referring to the PR audit trail, right? I'm sure the bug >> reporter >> is mistaken that the stores are coalesced. What happens is that two >> of the >> three stores are moved up above the first asm, but because of how the >> awk >> script cuts the generated code, that is not observed. > Thanks. That makes a lot more sense. > > >>> Also, if we go forward with your patch, the comment related to this >>> conditional needs to be fixed -- it still says "Flush pending lists >>> on jumps, >>> ...", but you've removed the jump check. >> >> Yeah, I'd say the comment is confusing both before and after the >> patch. >> Perhaps something like "Don't flush pending lists on speculation >> checks during >> selective scheduling" would be better. > Sounds good. Approved with that comment change. Thanks. You are right, I forgot to fix the comment before submitting. Is the patch also fine for 4.7/4.6? The problem is also present there. Andrey > > Thanks, > jeff
On 02/22/13 09:52, abel wrote: > > Thanks. You are right, I forgot to fix the comment before submitting. > > Is the patch also fine for 4.7/4.6? The problem is also present there. It's up to the release managers for 4.7/4.6. It certainly would have my blessing. jeff
On Fri, Feb 22, 2013 at 09:55:39AM -0700, Jeff Law wrote: > On 02/22/13 09:52, abel wrote: > > > >Thanks. You are right, I forgot to fix the comment before submitting. > > > >Is the patch also fine for 4.7/4.6? The problem is also present there. > It's up to the release managers for 4.7/4.6. It certainly would > have my blessing. It is ok, but please give it a week or two on the trunk first. Jakub
On 22.02.2013 17:30, Andrey Belevantsev wrote: > Hello, > > As found by Jakub and explained in the PR audit trail by Alexander, this > patch fixes the selective scheduler merge glitch of 2008 that added the > unnecessary JUMP_P check to the flush_pending_lists call. I have removed > the check and expanded the binary negation for clarity. > > The patch was tested on x86-64, ia64, and ppc64 to be safe. The patch > should be conservatively safe at this stage as it adds more flushes and > thus more dependencies to the scheduler. The original test is fixed, but I > don't know how to add the test checking assembly insns order to the testsuite. > > OK for trunk? > > Andrey > > 2012-02-22 Alexander Monakov <amonakov@ispras.ru> > Andrey Belevantsev <abel@ispras.ru> > > PR middle-end/56077 > * sched-deps.c (sched_analyze_insn): When reg_pending_barrier, > flush pending lists also on non-jumps. Now backported to 4.7 and 4.6. Andrey Index: gcc/ChangeLog =================================================================== *** gcc/ChangeLog (revision 197296) --- gcc/ChangeLog (revision 197297) *************** *** 1,3 **** --- 1,13 ---- + 2013-04-01 Andrey Belevantsev <abel@ispras.ru> + + Backport from mainline + 2013-02-25 Andrey Belevantsev <abel@ispras.ru> + Alexander Monakov <amonakov@ispras.ru> + + PR middle-end/56077 + * sched-deps.c (sched_analyze_insn): When reg_pending_barrier, + flush pending lists also on non-jumps. Adjust comment. + 2013-03-30 Gerald Pfeifer <gerald@pfeifer.com> * doc/invoke.texi (AVR Options): Tweak link for AVR-LibC user manual. Index: gcc/sched-deps.c =================================================================== *** gcc/sched-deps.c (revision 197296) --- gcc/sched-deps.c (revision 197297) *************** sched_analyze_insn (struct deps_desc *de *** 3262,3270 **** SET_REGNO_REG_SET (&deps->reg_last_in_use, i); } ! /* Flush pending lists on jumps, but not on speculative checks. */ ! if (JUMP_P (insn) && !(sel_sched_p () ! && sel_insn_is_speculation_check (insn))) flush_pending_lists (deps, insn, true, true); reg_pending_barrier = NOT_A_BARRIER; --- 3262,3270 ---- SET_REGNO_REG_SET (&deps->reg_last_in_use, i); } ! /* Don't flush pending lists on speculative checks for ! selective scheduling. */ ! if (!sel_sched_p () || !sel_insn_is_speculation_check (insn)) flush_pending_lists (deps, insn, true, true); reg_pending_barrier = NOT_A_BARRIER;
Index: gcc/sched-deps.c =================================================================== *** gcc/sched-deps.c (revision 196136) --- gcc/sched-deps.c (working copy) *************** sched_analyze_insn (struct deps_desc *de *** 3318,3325 **** } /* Flush pending lists on jumps, but not on speculative checks. */ ! if (JUMP_P (insn) && !(sel_sched_p () ! && sel_insn_is_speculation_check (insn))) flush_pending_lists (deps, insn, true, true); reg_pending_barrier = NOT_A_BARRIER; --- 3318,3324 ---- } /* Flush pending lists on jumps, but not on speculative checks. */ ! if (!sel_sched_p () || !sel_insn_is_speculation_check (insn)) flush_pending_lists (deps, insn, true, true); reg_pending_barrier = NOT_A_BARRIER;