Re: [RFC][PR middle-end/59285] builtin-unreachable-6 on ARM

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

On Tue, Nov 26, 2013 at 8:20 PM, Jeff Law wrote:
>
> The jump threading changes have exposed a latent bug on machines with
> conditional execution such as the ARM.
>
> Going into the last conditional execution pass we have:
>
> [ ... ]
> (insn 16 60 17 2 (set (reg:CC 100 cc)
>         (compare:CC (reg:SI 1 r1 [121])
>             (const_int 0 [0]))) j.c:14 226 {*arm_cmpsi_insn}
>      (expr_list:REG_DEAD (reg:SI 1 r1 [121])
>         (nil)))
> (jump_insn 17 16 19 2 (set (pc)
>         (if_then_else (ne (reg:CC 100 cc)
>                 (const_int 0 [0]))
>             (label_ref 25)
>             (pc))) j.c:14 235 {arm_cond_branch}
>      (expr_list:REG_DEAD (reg:CC 100 cc)
>         (int_list:REG_BR_PROB 5000 (nil)))
>  -> 25)
> ;;  succ:       4 [50.0%]
> ;;              3 [50.0%]  (FALLTHRU)
>
>
> ; basic block 3, loop depth 0, count 0, freq 7500, maybe hot
> ;; Invalid sum of incoming frequencies 5000, should be 7500
> ;;  prev block 2, next block 4, flags: (RTL)
> ;;  pred:       2 [50.0%]  (FALLTHRU)
> (note 19 17 18 3 [bb 3] NOTE_INSN_BASIC_BLOCK)
> (note/s 18 19 20 3 ("lab2") NOTE_INSN_DELETED_LABEL 3)
> ;;  succ:
>
>
> (note/s 20 18 21 ("lab") NOTE_INSN_DELETED_LABEL 4)
> (barrier 21 20 25)
>
>
> ;; basic block 4, loop depth 0, count 0, freq 0
> ;; Invalid sum of incoming frequencies 5000, should be 0
> ;;  prev block 3, next block 5, flags: (RTL)
> ;;  pred:       2 [50.0%]
> (code_label 25 21 26 4 2 "" [1 uses])
> (note 26 25 27 4 [bb 4] NOTE_INSN_BASIC_BLOCK)
> ;;  succ:       5 [100.0%]  (FALLTHRU)
>
>
> ;; basic block 5, loop depth 0, count 0, freq 2500
> ;;  prev block 4, next block 1, flags: (RTL)
> ;;  pred:       4 [100.0%]  (FALLTHRU)
> ;;              5 [100.0%]  (DFS_BACK)
> (code_label 27 26 28 5 5 "" [1 uses])
> (note 28 27 56 5 [bb 5] NOTE_INSN_BASIC_BLOCK)
> (jump_insn 56 28 57 5 (set (pc)
>         (label_ref 27)) 249 {*arm_jump}
>      (nil)
>  -> 27)
> ;;  succ:       5 [100.0%]  (DFS_BACK)
>
> (barrier 57 56 58)
> (note 58 57 0 NOTE_INSN_DELETED)
>
>
> Note carefully BB3 which is the result of a __builtin_unreachable in the
> original source.  It has no code, no successors and a trailing barrier (all
> as expected).
>
> ce3 comes along and creates this mess:
>
> [ ... ]
>
> (insn 16 60 18 2 (set (reg:CC 100 cc)
>         (compare:CC (reg:SI 1 r1 [121])
>             (const_int 0 [0]))) j.c:14 226 {*arm_cmpsi_insn}
>      (expr_list:REG_DEAD (reg:SI 1 r1 [121])
>         (nil)))
> (note/s 18 16 20 2 ("lab2") NOTE_INSN_DELETED_LABEL 3)
> ;;  succ:       5 [100.0%]  (FALLTHRU)
>
> (note/s 20 18 21 ("lab") NOTE_INSN_DELETED_LABEL 4)
> (barrier 21 20 27)
>
> ;; basic block 5, loop depth 0, count 0, freq 2500
> ;; Invalid sum of incoming frequencies 12500, should be 2500
> ;;  prev block 2, next block 1, flags: (RTL)
> ;;  pred:       2 [100.0%]  (FALLTHRU)
> ;;              5 [100.0%]  (DFS_BACK)
> (code_label 27 21 28 5 5 "" [1 uses])
> (note 28 27 56 5 [bb 5] NOTE_INSN_BASIC_BLOCK)
> (jump_insn 56 28 57 5 (set (pc)
>         (label_ref 27)) 249 {*arm_jump}
>      (nil)
>  -> 27)
> ;;  succ:       5 [100.0%]  (DFS_BACK)
>
> (barrier 57 56 58)
> (note 58 57 0 NOTE_INSN_DELETED)
>
>
> Note how it's removed the conditional jump at the end of bb2.  BB2 still has
> an outgoing edge, to BB5, but there's a barrier after BB2.
>
> This, of course, trips a checking failure.

I've been running into more of these.

<rant>
BARRIERs should not exist as long as we have a CFG. The CFG has all
the information.
</rant>

This is one of my goals for the next stage1.


> ISTM that when we merge blocks due to if-conversion of this code of code,
> we're always going to have an outgoing edge from the merged block (sanity
> check, right?)
>
> Something along these lines I think.  However, I'm not at all familiar with
> this code, so some sanity checking would be greatly appreciated.

I'm surprised if-conversion applies here at all. You basically have an
incomplete diamond and that's only partially handled by ifcvt.

There is this comment:

 /* If the THEN block has no successors, conditional execution can still
     make a conditional call.  Don't do this unless the ELSE block has
     only one incoming edge -- the CFG manipulation is too ugly otherwise.
     Check for the last insn of the THEN block being an indirect jump, which
     is listed as not having any successors, but confuses the rest of the CE
     code processing.  ??? we should fix this in the future.  */

and it is assumed that there is a noreturn call in the block without
successors. In other words, the THEN block is really a dead end in the
CFG and it cannot be merged away unless the noreturn call is
if-converted away. In the case of builtin_unreachable, obviously there
isn't a call, but I'm guessing your test case goes through this path
anyway.

I believe the proper fix would be to not recognize this as an
if-conversion block candidate in cond_exec_find_if_block.

Ciao!
Steven