On Sun, Nov 17, 2013 at 7:48 AM, Jeff Law wrote:
>
> * combine.c (try_combine): If we have created an unconditional trap,
> make sure to fixup the insn stream & CFG appropriately.
>
> diff --git a/gcc/combine.c b/gcc/combine.c
> index 13f5e29..b3d20f2 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -4348,6 +4348,37 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx i0, int
> *new_direct_jump_p,
> update_cfg_for_uncondjump (undobuf.other_insn);
> }
>
> + /* If we might have created an unconditional trap, then we have
> + cleanup work to do.
> +
> + The fundamental problem is a conditional trap is not considered
> + control flow altering, while an unconditional trap is considered
> + control flow altering.
> +
> + So while we could have a conditional trap in the middle of a block
> + we can not have an unconditional trap in the middle of a block. */
> + if (GET_CODE (i3) == INSN
> + && GET_CODE (PATTERN (i3)) == TRAP_IF
> + && XEXP (PATTERN (i3), 0) == const1_rtx)
TRAP_CONDITION (PATTERN (i3)) == const1_rtx
But shouldn't the check be on const_true_rtx? Or does combine put a
const1_rtx there?
> + {
> + basic_block bb = BLOCK_FOR_INSN (i3);
> + rtx last = get_last_bb_insn (bb);
This won't work, get_last_bb_insn() is intended to be used only in
cfgrtl mode and "combine" works in cfglayout mode. If you use it in
cfglayout mode on a block that ends in a tablejump, you get back the
JUMP_TABLE_DATA insn that is in BB_FOOTER and there is no NEXT_INSN
path from BB_END to any insns in the footer.
rtx last = BB_END (bb);
Any dead jump tables will be dealt with later.
> + /* First remove all the insns after the trap. */
> + if (i3 != last)
> + delete_insn_chain (NEXT_INSN (i3), last, true);
> +
> + /* And ensure there's no outgoing edges anymore. */
> + while (EDGE_COUNT (bb->succs) > 0)
> + remove_edge (EDGE_SUCC (bb, 0));
Alternatively, you could do "split_block (bb, i3);" and let cfgcleanup
deal with the new, unreachable basic block.
> + /* And ensure cfglayout knows this block does not fall through. */
> + emit_barrier_after_bb (bb);
Bah... Emitting the barrier is necessary here because
fixup_reorder_chain doesn't handle cases where a basic block is a dead
end. That is actually a bug in fixup_reorder_chain: Other passes could
create dead ends in the CFG in cfglayout mode and not emit a barrier
into BB_FOOTER, and fixup_reorder_chain wouldn't be able to handle
that (resulting in verify_flow_info failure).
fixup_reorder_chain should emit a BARRIER if a block has no successor edges.
(It's a general short-comming of cfglayout mode that barriers are
still there at all. Ideally all barriers would be removed going into
cfglayout mode, and fixup_reorder_chain would put them back where
necessary. That would simplify the job of updating the CFG elsewhere
in the compiler, e.g. update_cfg_for_uncondjump)
> + /* Not exactly true, but gets the effect we want. */
> + *new_direct_jump_p = 1;
> + }
> +
> /* A noop might also need cleaning up of CFG, if it comes from the
> simplification of a jump. */
> if (JUMP_P (i3)
>
Would you mind if I try spend some time making conditional traps be
control flow insns? It should make all of this a little bit less ugly.
And I have no fish to fry at all :-) Give me a week or two please, to
see if I can figure out those issues you've been running into.
Ciao!
Steven
