Patchwork Cleanup the CFG after pro_and_epilogue pass

login
register
mail settings
Submitter Steven Bosscher
Date May 17, 2013, 7:49 p.m.
Message ID <CABu31nOj1yqt3nwWYqDHBBtZiJQ3GM9brnBjcmA3+paucXXKtw@mail.gmail.com>
Download mbox | patch
Permalink /patch/244696/
State New
Headers show

Comments

Steven Bosscher - May 17, 2013, 7:49 p.m.
Hello,

Trying to dump the CFG as a graph fails after the pro_and_epilogue
pass because it leaves unreachable basic blocks. This patch fixes that
issue.

Will commit sometime next week if no-one objects.

Ciao!
Steven


        * function.c (rest_of_handle_thread_prologue_and_epilogue):
        Cleanup the CFG after thread_prologue_and_epilogue_insns.
Jeff Law - May 17, 2013, 7:52 p.m.
On 05/17/2013 01:49 PM, Steven Bosscher wrote:
> Hello,
>
> Trying to dump the CFG as a graph fails after the pro_and_epilogue
> pass because it leaves unreachable basic blocks. This patch fixes that
> issue.
>
> Will commit sometime next week if no-one objects.
>
> Ciao!
> Steven
>
>
>          * function.c (rest_of_handle_thread_prologue_and_epilogue):
>          Cleanup the CFG after thread_prologue_and_epilogue_insns.
?!?

Under what conditions does threading the prologue/epilogue make code 
unreachable?

Also note that commit after X days if no-one objects is not the projects 
policy.  Please don't take such liberties.


Jeff
Steven Bosscher - May 17, 2013, 8:53 p.m.
On Fri, May 17, 2013 at 9:52 PM, Jeff Law <law@redhat.com> wrote:
> On 05/17/2013 01:49 PM, Steven Bosscher wrote:
>>
>> Hello,
>>
>> Trying to dump the CFG as a graph fails after the pro_and_epilogue
>> pass because it leaves unreachable basic blocks. This patch fixes that
>> issue.
>>
>> Will commit sometime next week if no-one objects.
>>
>> Ciao!
>> Steven
>>
>>
>>          * function.c (rest_of_handle_thread_prologue_and_epilogue):
>>          Cleanup the CFG after thread_prologue_and_epilogue_insns.
>
> ?!?
>
> Under what conditions does threading the prologue/epilogue make code
> unreachable?

Compiling testsuite/gcc.target/i386/pad-10.c with -fdump-rtl-all-graph.

Just before rest_of_handle_thread_prologue_and_epilogue we have:

Breakpoint 2, rest_of_handle_thread_prologue_and_epilogue () at
../../trunk/gcc/function.c:6969
6969    {
(gdb) p brief_dump_cfg(stderr,-1)
;; basic block 2, loop depth 0, count 0, freq 10000, maybe hot
;;  prev block 0, next block 3, flags: (REACHABLE, RTL, MODIFIED)
;;  pred:       ENTRY [100.0%]  (FALLTHRU)
;;  succ:       3 [9.2%]  (FALLTHRU)
;;              4 [90.8%]
;; basic block 3, loop depth 0, count 0, freq 922, maybe hot
;;  prev block 2, next block 4, flags: (REACHABLE, RTL, MODIFIED)
;;  pred:       2 [9.2%]  (FALLTHRU)
;;  succ:       4 [100.0%]  (FALLTHRU)
;; basic block 4, loop depth 0, count 0, freq 10000, maybe hot
;;  prev block 3, next block 1, flags: (REACHABLE, RTL, MODIFIED)
;;  pred:       3 [100.0%]  (FALLTHRU)
;;              2 [90.8%]
;;  succ:       EXIT [100.0%]  (FALLTHRU)
$1 = void
(gdb) p debug_bb_n(2)
(note 6 1 4 2 [bb 2] NOTE_INSN_BASIC_BLOCK)
(note 4 6 31 2 NOTE_INSN_FUNCTION_BEG)
(insn 31 4 17 2 (set (reg:SI 0 ax [orig:59 D.1775 ] [59])
        (reg/v:SI 4 si [orig:62 x ] [62]))
testsuite/gcc.target/i386/pad-10.c:18 85 {*movsi_internal}
     (nil))
(insn 17 31 8 2 (parallel [
            (set (reg:SI 0 ax [orig:59 D.1775 ] [59])
                (minus:SI (reg:SI 0 ax [orig:59 D.1775 ] [59])
                    (reg/v:SI 5 di [orig:61 z ] [61])))
            (clobber (reg:CC 17 flags))
        ]) testsuite/gcc.target/i386/pad-10.c:18 295 {*subsi_1}
     (nil))
(insn 8 17 9 2 (set (reg:CCZ 17 flags)
        (compare:CCZ (reg/v:SI 4 si [orig:62 x ] [62])
            (const_int 1 [0x1])))
testsuite/gcc.target/i386/pad-10.c:12 7 {*cmpsi_1}
     (expr_list:REG_DEAD (reg/v:SI 4 si [orig:62 x ] [62])
        (nil)))
(jump_insn 9 8 10 2 (set (pc)
        (if_then_else (ne (reg:CCZ 17 flags)
                (const_int 0 [0]))
            (label_ref:DI 18)
            (pc))) testsuite/gcc.target/i386/pad-10.c:12 602 {*jcc_1}
     (expr_list:REG_BR_PROB (const_int 9078 [0x2376])
        (nil))
 -> 18)

$2 = (basic_block_def *) 0x7ffff6224410
(gdb) p debug_bb_n(3)
(note 10 9 33 3 [bb 3] NOTE_INSN_BASIC_BLOCK)
(insn 33 10 11 3 (set (mem/c:SI (plus:DI (reg/f:DI 7 sp)
                (const_int 12 [0xc])) [2 %sfp+-4 S4 A32])
        (reg/v:SI 5 di [orig:61 z ] [61])) 85 {*movsi_internal}
     (expr_list:REG_DEAD (reg/v:SI 5 di [orig:61 z ] [61])
        (nil)))
(insn 11 33 12 3 (set (reg:QI 0 ax)
        (const_int 0 [0])) testsuite/gcc.target/i386/pad-10.c:14 87
{*movqi_internal}
     (nil))
(call_insn 12 11 34 3 (call (mem:QI (symbol_ref:DI ("bar") [flags
0x41]  <function_decl 0x7ffff6225800 bar>) [0 bar S1 A8])
        (const_int 0 [0])) testsuite/gcc.target/i386/pad-10.c:14 646 {*call}
     (expr_list:REG_DEAD (reg:QI 0 ax)
        (nil))
    (expr_list:REG_DEP_TRUE (use (reg:QI 0 ax))
        (nil)))
(insn 34 12 5 3 (set (reg/v:SI 5 di [orig:61 z ] [61])
        (mem/c:SI (plus:DI (reg/f:DI 7 sp)
                (const_int 12 [0xc])) [2 %sfp+-4 S4 A32]))
testsuite/gcc.target/i386/pad-10.c:15 85 {*movsi_internal}
     (expr_list:REG_DEAD (reg:SI 65)
        (nil)))
(insn 5 34 18 3 (set (reg:SI 0 ax [orig:59 D.1775 ] [59])
        (reg/v:SI 5 di [orig:61 z ] [61]))
testsuite/gcc.target/i386/pad-10.c:15 85 {*movsi_internal}
     (expr_list:REG_DEAD (reg/v:SI 5 di [orig:61 z ] [61])
        (nil)))

$3 = (basic_block_def *) 0x7ffff6224208
(gdb) p debug_bb_n(4)
(code_label 18 5 19 4 3 "" [1 uses])
(note 19 18 27 4 [bb 4] NOTE_INSN_BASIC_BLOCK)
(insn 27 19 0 4 (use (reg/i:SI 0 ax)) testsuite/gcc.target/i386/pad-10.c:19 -1
     (nil))

$4 = (basic_block_def *) 0x7ffff62242d8



Continuing from here leads to an ice in the graph dumper:

(gdb) cont
Continuing.
Breakpoint 1, internal_error (gmsgid=gmsgid@entry=0x119725e "in %s, at
%s:%d") at ../../trunk/gcc/diagnostic.c:1091
1091    bool
(gdb) bt 10
#0  internal_error (gmsgid=gmsgid@entry=0x119725e "in %s, at %s:%d")
at ../../trunk/gcc/diagnostic.c:1091
#1  0x0000000000dfda24 in fancy_abort (file=<optimized out>, line=869,
function=0xee3e60 <pre_and_rev_post_order_compute(int*, int*,
bool)::__FUNCTION__> "pre_and_rev_post_order_compute") at
../../trunk/gcc/diagnostic.c:1151
#2  0x000000000061dbab in pre_and_rev_post_order_compute
(pre_order=0x0, rev_post_order=0x1627850,
include_entry_exit=<optimized out>) at ../../trunk/gcc/cfganal.c:869
#3  0x0000000000d28d22 in draw_cfg_nodes_no_loops (fun=<optimized
out>, fun=<optimized out>, pp=0x15a4f20
<init_graph_slim_pretty_print(_IO_FILE*)::graph_slim_pp>) at
../../trunk/gcc/graph.c:183
#4  draw_cfg_nodes (fun=0x7ffff6114140, pp=0x15a4f20
<init_graph_slim_pretty_print(_IO_FILE*)::graph_slim_pp>) at
../../trunk/gcc/graph.c:265
#5  print_graph_cfg (base=<optimized out>, fun=0x7ffff6114140) at
../../trunk/gcc/graph.c:306
#6  0x000000000087a330 in execute_one_pass (pass=pass@entry=0x14f6b20
<pass_thread_prologue_and_epilogue>) at ../../trunk/gcc/passes.c:2349
#7  0x000000000087a6a5 in execute_pass_list (pass=0x14f6b20
<pass_thread_prologue_and_epilogue>) at ../../trunk/gcc/passes.c:2378
#8  0x000000000087a6b7 in execute_pass_list (pass=0x14f77a0
<pass_postreload>) at ../../trunk/gcc/passes.c:2379
#9  0x000000000087a6b7 in execute_pass_list (pass=0x14f7800
<pass_rest_of_compilation>) at ../../trunk/gcc/passes.c:2379
(More stack frames follow...)
(gdb) p brief_dump_cfg(stderr,-1)
;; basic block 2, loop depth 0, count 0, freq 10000, maybe hot
;;  prev block 0, next block 3, flags: (REACHABLE, RTL, MODIFIED)
;;  pred:       ENTRY [100.0%]  (FALLTHRU)
;;  succ:       3 [9.2%]  (FALLTHRU)
;;              6 [90.8%]
;; basic block 3, loop depth 0, count 0, freq 922, maybe hot
;;  prev block 2, next block 4, flags: (REACHABLE, RTL, MODIFIED)
;;  pred:       2 [9.2%]  (FALLTHRU)
;;  succ:       4 [100.0%]  (FALLTHRU)
;; basic block 4, loop depth 0, count 0, freq 922, maybe hot
;;  prev block 3, next block 6, flags: (REACHABLE, RTL, MODIFIED)
;;  pred:       3 [100.0%]  (FALLTHRU)
;;  succ:       6 [100.0%]  (FALLTHRU)
;; basic block 6, loop depth 0, count 0, freq 922, maybe hot
;; Invalid sum of incoming frequencies 10000, should be 922
;;  prev block 4, next block 5, flags: (NEW, RTL, MODIFIED)
;;  pred:       4 [100.0%]  (FALLTHRU)
;;              2 [90.8%]
;;  succ:       EXIT [100.0%]
;; basic block 5, loop depth 0, count 0, freq 9078, maybe hot
;; Invalid sum of incoming frequencies 0, should be 9078
;;  prev block 6, next block 1, flags: (NEW, RTL, MODIFIED)
;;  pred:
;;  succ:       EXIT [100.0%]  (FAKE)
$5 = void
(gdb) # Note basic block 5 has no predecessors
(gdb) p debug_bb_n(4)
(code_label 18 5 19 4 3 "" [0 uses])
(note 19 18 27 4 [bb 4] NOTE_INSN_BASIC_BLOCK)
(insn 27 19 45 4 (use (reg/i:SI 0 ax)) testsuite/gcc.target/i386/pad-10.c:19 -1
     (nil))
(note 45 27 46 4 NOTE_INSN_EPILOGUE_BEG)
(insn/f 46 45 50 4 (parallel [
            (set (reg/f:DI 7 sp)
                (plus:DI (reg/f:DI 7 sp)
                    (const_int 24 [0x18])))
            (clobber (reg:CC 17 flags))
            (clobber (mem:BLK (scratch) [0 A8]))
        ]) testsuite/gcc.target/i386/pad-10.c:19 -1
     (expr_list:REG_CFA_ADJUST_CFA (set (reg/f:DI 7 sp)
            (plus:DI (reg/f:DI 7 sp)
                (const_int 24 [0x18])))
        (nil)))

$6 = (basic_block_def *) 0x7ffff62242d8
(gdb) p debug_bb_n(5)
(code_label 44 48 39 5 5 "" [0 uses])
(note 39 44 43 5 [bb 5] NOTE_INSN_BASIC_BLOCK)
(insn 43 39 41 5 (use (reg/i:SI 0 ax)) testsuite/gcc.target/i386/pad-10.c:19 -1
     (nil))

$7 = (basic_block_def *) 0x7ffff6224548
(gdb)



What's happened, is that emitting the epilogue at the end of basic
block 4 (with a barrier at the end) has made the use insn 43
unreachable. The epilogue emitter is not CFG aware, so
thread_prologue_and_epilogue_insns has to use
find_many_sub_basic_blocks to split blocks it has emitted insns into.
find_many_sub_basic_blocks splits basic block 4 and creates basic
block 5. The user of find_many_sub_basic_blocks is responsible for
cleaning up after h{im,er}self, but thread_prologue_and_epilogue_insns
doesn't currently do so. My patch corrects this.

I put the cleanup_cfg(0) in
rest_of_handle_thread_prologue_and_epilogue because I suspect there
are other ways in which thread_prologue_and_epilogue_insns can leave
unreachable blocks, especially after shrink-wrapping.


> Also note that commit after X days if no-one objects is not the projects
> policy.  Please don't take such liberties.

This reaction seems to me like an unnecessary and disappointing slap
on the wrist. Project policy does allow committing obvious patches
without review. Perhaps I should just use that policy and not give
people the chance to comment? I would think you would also know by now
that I know all CFG related code as well as anyone, and, frankly,
probably better than even you.

Ciao!
Steven
Jeff Law - May 17, 2013, 9:16 p.m.
On 05/17/2013 02:53 PM, Steven Bosscher wrote:
> On Fri, May 17, 2013 at 9:52 PM, Jeff Law <law@redhat.com> wrote:
>> On 05/17/2013 01:49 PM, Steven Bosscher wrote:
>>>
>>> Hello,
>>>
>>> Trying to dump the CFG as a graph fails after the pro_and_epilogue
>>> pass because it leaves unreachable basic blocks. This patch fixes that
>>> issue.
>>>
>>> Will commit sometime next week if no-one objects.
>>>
>>> Ciao!
>>> Steven
>>>
>>>
>>>           * function.c (rest_of_handle_thread_prologue_and_epilogue):
>>>           Cleanup the CFG after thread_prologue_and_epilogue_insns.
>>
>> ?!?
>>
>> Under what conditions does threading the prologue/epilogue make code
>> unreachable?
>
> Compiling testsuite/gcc.target/i386/pad-10.c with -fdump-rtl-all-graph.
>
> Just before rest_of_handle_thread_prologue_and_epilogue we have:
Thanks.


>
>
> What's happened, is that emitting the epilogue at the end of basic
> block 4 (with a barrier at the end) has made the use insn 43
> unreachable.
But from the description you've given, it appears that the epilogue 
itself has unreachable code, and that shouldn't be happening.  If you 
think it can happen by way of shrink-wrapping, I'd like to see the analysis.

What does the epilogue itself look like before threading it into the 
insn stream?

> This reaction seems to me like an unnecessary and disappointing slap
> on the wrist. Project policy does allow committing obvious patches
> without review. Perhaps I should just use that policy and not give
> people the chance to comment? I would think you would also know by now
> that I know all CFG related code as well as anyone, and, frankly,
> probably better than even you.
You do not make project policy.  It's that simple.

Yes we have a obvious patch policy, but this certainly doesn't fall 
under that policy.  I would frown upon you using the obvious patch path 
to bypass existing policy for patch review.

If you want commit without review privileges, ask for someone to sponsor 
your request an the steering committee will vote on it.

jeff
Steven Bosscher - May 17, 2013, 9:53 p.m.
On Fri, May 17, 2013 at 11:16 PM, Jeff Law wrote:
>> What's happened, is that emitting the epilogue at the end of basic
>> block 4 (with a barrier at the end) has made the use insn 43
>> unreachable.
>
> But from the description you've given, it appears that the epilogue itself
> has unreachable code, and that shouldn't be happening.  If you think it can
> happen by way of shrink-wrapping, I'd like to see the analysis.

It is not the epilogue itself but the way shrink-wrapping emits it.
The block that is unreachable has its last predecessor edge removed in
function.c:6607:

6607              redirect_edge_and_branch_force (e, *pdest_bb);

I haven't looked at how the shrink-wrapping code works exactly. It's
Bernd's code, so perhaps he can have a look. This is now PR57320.

Ciao!
Steven
Steven Bosscher - May 17, 2013, 9:53 p.m.
On Fri, May 17, 2013 at 9:49 PM, Steven Bosscher wrote:
> Will commit sometime next week if no-one objects.

Obviously there are objections.
Patch withdrawn.

Ciao!
Steven
Jeff Law - May 17, 2013, 9:59 p.m.
On 05/17/2013 03:53 PM, Steven Bosscher wrote:
> On Fri, May 17, 2013 at 11:16 PM, Jeff Law wrote:
>>> What's happened, is that emitting the epilogue at the end of basic
>>> block 4 (with a barrier at the end) has made the use insn 43
>>> unreachable.
>>
>> But from the description you've given, it appears that the epilogue itself
>> has unreachable code, and that shouldn't be happening.  If you think it can
>> happen by way of shrink-wrapping, I'd like to see the analysis.
>
> It is not the epilogue itself but the way shrink-wrapping emits it.
> The block that is unreachable has its last predecessor edge removed in
> function.c:6607:
>
> 6607              redirect_edge_and_branch_force (e, *pdest_bb);
>
> I haven't looked at how the shrink-wrapping code works exactly. It's
> Bernd's code, so perhaps he can have a look. This is now PR57320.
OK.  Let's go with your patch then.  Approved with a comment that 
shrink-wrapping can result in unreachable edges in the epilogue.

jeff

Patch

Index: function.c
===================================================================
--- function.c  (revision 199028)
+++ function.c  (working copy)
@@ -6976,6 +6976,9 @@  rest_of_handle_thread_prologue_and_epilo
      scheduling to operate in the epilogue.  */
   thread_prologue_and_epilogue_insns ();

+  /* The prologue and epilogue may have made some blocks unreachable.  */
+  cleanup_cfg (0);
+
   /* The stack usage info is finalized during prologue expansion.  */
   if (flag_stack_usage_info)
     output_stack_usage ();