Re: [RFA][PATCH]Fix 59019

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

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