Re: [patch] Cleanup the CFG after pro_and_epilogue pass

[Steven Bosscher <stevenb dot gcc at gmail dot com>]

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