| Message ID | CABu31nOj1yqt3nwWYqDHBBtZiJQ3GM9brnBjcmA3+paucXXKtw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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 ();
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.