Patchwork Fix PR 56077

login
register
mail settings
Submitter Andrey Belevantsev
Date Feb. 22, 2013, 1:30 p.m.
Message ID <512772F5.3040007@ispras.ru>
Download mbox | patch
Permalink /patch/222531/
State New
Headers show

Comments

Andrey Belevantsev - Feb. 22, 2013, 1:30 p.m.
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.
Jeff Law - Feb. 22, 2013, 1:56 p.m.
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
Alexander Monakov - Feb. 22, 2013, 2:16 p.m.
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
Jeff Law - Feb. 22, 2013, 2:21 p.m.
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
Andrey Belevantsev - Feb. 22, 2013, 4:52 p.m.
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
Jeff Law - Feb. 22, 2013, 4:55 p.m.
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
Jakub Jelinek - Feb. 22, 2013, 5:15 p.m.
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
Andrey Belevantsev - April 1, 2013, 8:38 a.m.
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;

Patch

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;