[PR47036] sel-sched: don't attempt to move up unconditional jumps

[PR47036] sel-sched: don't attempt to move up unconditional jumps

[Alexander Monakov]

Hi,

This patch drops bits of support for moving up unconditional jumps.  The rest
of the scheduler does not handle it anyway, and most of the time dependencies
would prohibit such motion (in the testcase it's possible because other insns
are nops).

I've verified that the patch does not affect code generation on several cc1 .i
files at -O3 (targeting ia64).  Bootstrapped on x86-64-linux, ia64-linux
(testsuite running).  OK for trunk?


2010-12-22  Alexander Monakov  <amonakov@ispras.ru>

	PR rtl-optimization/47036
	* sel-sched-ir.c (fallthru_bb_of_jump): Remove special support for
	unconditional jumps.
	* sel-sched.c (moveup_expr): Ditto.

testsuite:
	* g++.dg/opt/pr47036.C: New.

diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index 468dfd7..de40ba0 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -4441,9 +4441,6 @@ fallthru_bb_of_jump (rtx jump)
   if (!JUMP_P (jump))
     return NULL;
 
-  if (any_uncondjump_p (jump))
-    return single_succ (BLOCK_FOR_INSN (jump));
-
   if (!any_condjump_p (jump))
     return NULL;
 
diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c
index 3b5603c..b5c9e57 100644
--- a/gcc/sel-sched.c
+++ b/gcc/sel-sched.c
@@ -2171,10 +2171,8 @@ moveup_expr (expr_t expr, insn_t through_insn, bool inside_insn_group,
               || ! in_current_region_p (fallthru_bb))
             return MOVEUP_EXPR_NULL;
 
-          /* And it should be mutually exclusive with through_insn, or
-             be an unconditional jump.  */
-          if (! any_uncondjump_p (insn)
-              && ! sched_insns_conditions_mutex_p (insn, through_insn)
+          /* And it should be mutually exclusive with through_insn.  */
+          if (! sched_insns_conditions_mutex_p (insn, through_insn)
 	      && ! DEBUG_INSN_P (through_insn))
             return MOVEUP_EXPR_NULL;
         }
diff --git a/gcc/testsuite/g++.dg/opt/pr47036.C b/gcc/testsuite/g++.dg/opt/pr47036.C
index e69de29..d6d5adc 100644
--- a/gcc/testsuite/g++.dg/opt/pr47036.C
+++ b/gcc/testsuite/g++.dg/opt/pr47036.C
@@ -0,0 +1,10 @@
+// { dg-do compile { target powerpc*-*-* ia64-*-* x86_64-*-* } }
+// { dg-options "-fschedule-insns -fselective-scheduling -fno-dce" }
+
+
+void foo ()
+{
+  for (;;)
+    for (;;({break;}));
+}
+

Re: [PR47036] sel-sched: don't attempt to move up unconditional jumps

[Andrey Belevantsev]

On 22.12.2010 20:57, Alexander Monakov wrote:
> Hi,
>
> This patch drops bits of support for moving up unconditional jumps.  The rest
> of the scheduler does not handle it anyway, and most of the time dependencies
> would prohibit such motion (in the testcase it's possible because other insns
> are nops).
While I'm happy with this patch, I would like to note that I'm rather tired 
of fixing various corner cases that are only seen in the scheduler when 
running it with -O0.  I do believe that we need to force cleanup_cfg and 
DCE when not optimizing and stop caring.

>
> I've verified that the patch does not affect code generation on several cc1 .i
> files at -O3 (targeting ia64).  Bootstrapped on x86-64-linux, ia64-linux
> (testsuite running).  OK for trunk?
I would also suggest to go further and check codegen differences on SPEC2k.

Andrey

Re: [PR47036] sel-sched: don't attempt to move up unconditional jumps

[Vladimir Makarov]

On 12/22/2010 04:02 PM, Andrey Belevantsev wrote:
> On 22.12.2010 20:57, Alexander Monakov wrote:
>> Hi,
>>
>> This patch drops bits of support for moving up unconditional jumps.  
>> The rest
>> of the scheduler does not handle it anyway, and most of the time 
>> dependencies
>> would prohibit such motion (in the testcase it's possible because 
>> other insns
>> are nops).
> While I'm happy with this patch, I would like to note that I'm rather 
> tired of fixing various corner cases that are only seen in the 
> scheduler when running it with -O0.  I do believe that we need to 
> force cleanup_cfg and DCE when not optimizing and stop caring.
>
I don't think it is a good solution, Andrey.  It is practically the same 
solution as preventing insn scheduling for -O0.  While the passes are 
separate and have options to switch them on/off, the pass should work 
independently of the other passes work.
>>
>> I've verified that the patch does not affect code generation on 
>> several cc1 .i
>> files at -O3 (targeting ia64).  Bootstrapped on x86-64-linux, ia64-linux
>> (testsuite running).  OK for trunk?
> I would also suggest to go further and check codegen differences on 
> SPEC2k.
>
Yes, it would be interesting to see differences.

Re: [PR47036] sel-sched: don't attempt to move up unconditional jumps

[Vladimir Makarov]

On 12/22/2010 12:57 PM, Alexander Monakov wrote:
> Hi,
>
> This patch drops bits of support for moving up unconditional jumps.  The rest
> of the scheduler does not handle it anyway, and most of the time dependencies
> would prohibit such motion (in the testcase it's possible because other insns
> are nops).
>
> I've verified that the patch does not affect code generation on several cc1 .i
> files at -O3 (targeting ia64).  Bootstrapped on x86-64-linux, ia64-linux
> (testsuite running).  OK for trunk?
>
Ok.  Thanks for fixing the bug.

> 2010-12-22  Alexander Monakov<amonakov@ispras.ru>
>
> 	PR rtl-optimization/47036
> 	* sel-sched-ir.c (fallthru_bb_of_jump): Remove special support for
> 	unconditional jumps.
> 	* sel-sched.c (moveup_expr): Ditto.
>
> testsuite:
> 	* g++.dg/opt/pr47036.C: New.

Re: [PR47036] sel-sched: don't attempt to move up unconditional jumps

[Andrey Belevantsev]

On 23.12.2010 19:56, Vladimir Makarov wrote:
> On 12/22/2010 04:02 PM, Andrey Belevantsev wrote:
>> On 22.12.2010 20:57, Alexander Monakov wrote:
>>> Hi,
>>>
>>> This patch drops bits of support for moving up unconditional jumps. The
>>> rest
>>> of the scheduler does not handle it anyway, and most of the time
>>> dependencies
>>> would prohibit such motion (in the testcase it's possible because other
>>> insns
>>> are nops).
>> While I'm happy with this patch, I would like to note that I'm rather
>> tired of fixing various corner cases that are only seen in the scheduler
>> when running it with -O0. I do believe that we need to force cleanup_cfg
>> and DCE when not optimizing and stop caring.
>>
> I don't think it is a good solution, Andrey. It is practically the same
> solution as preventing insn scheduling for -O0. While the passes are
> separate and have options to switch them on/off, the pass should work
> independently of the other passes work.
I guess you and Alexander are right, and I'm just tired and need a vacation 
:)  Happy holidays, Vlad, and thanks for bearing with our patches, we kept 
you busy this year.

Andrey