[RFA] Fix 59019

59019 is currently latent on the trunk, but it's likely to fail again at 
some point.

The problem we have is combine transforms a conditional trap into an 
unconditional trap.

conditional traps are not considered control flow insns, but 
unconditional traps are.

Thus, if we turn a conditional trap in the middle of a block into an 
unconditional trap, we end up with a control flow insn in the middle of 
a block and trip a checking assert.

This is, IMHO, a bandaid.  The inconsistency is amazingly annoying.  But 
I've got bigger fish to fry and I was unhappy with the number of issues 
I was running into when I tried to make conditional traps control flow 
insns.

Basically when we see an unconditional trap after we've done combining, 
we remove all the insns after the trap to the end of the block, delete 
the block's outgoing edges and emit a barrier into the block's footer.

It's similar in spirit to the cleanups we do for other situations.

Bootstrapped on ia64 with a hack installed to make this situation more 
likely to arise.

Ok for the trunk if it passes a bootstrap & regression test on 
x86_64-unknown-linux-gnu?

Jeff
* combine.c (try_combine): If we have created an unconditional trap,
	make sure to fixup the insn stream & CFG appropriately.

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

On 11/17/13 04:28, Steven Bosscher wrote:
>
> TRAP_CONDITION (PATTERN (i3)) == const1_rtx
>
> But shouldn't the check be on const_true_rtx? Or does combine put a
> const1_rtx there?
I took const1_rtx from control_flow_insn_p.  That's ultimately what we 
need to be consistent with.


>
>
>> +    {
>> +      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.
OK.

>
>
>> +      /* 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.
My first iteration split the block and let cfgcleanup take care of the 
rest.  That seemed wasteful when all we really need to do is zap the 
trailing instructions.

>
>
>
>> +      /* 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).
Umm, no.  Failure to emit the barrier will result in a checking failure. 
  Been there, done that.

>>
>
> 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.
Feel free.  I'm not terribly concerned about this issue right now.

To trigger use the test in 59019 with an itanic cross compiler and 
comment out these two lines from gimple-ssa-isolate-paths.c:


       TREE_THIS_VOLATILE (op) = 1;
       TREE_SIDE_EFFECTS (op) = 1;



jeff

On Mon, Nov 18, 2013 at 5:36 AM, Jeff Law <law@redhat.com> wrote:
> On 11/17/13 04:28, Steven Bosscher wrote:
>>
>>
>> TRAP_CONDITION (PATTERN (i3)) == const1_rtx
>>
>> But shouldn't the check be on const_true_rtx? Or does combine put a
>> const1_rtx there?
>
> I took const1_rtx from control_flow_insn_p.  That's ultimately what we need
> to be consistent with.

Hmm, I agree but look at the test in ifcvt.c...

>> 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).
>
> Umm, no.  Failure to emit the barrier will result in a checking failure.
> Been there, done that.

Read my comment again: "... is necessary ..." and "...
fixup_reorder_chain should emit a BARRIER ...".

So yes, you need to emit that barrier with GCC as-is. But in a
perfect, ideal world where everyone is happy all the time, the sun
always shines, lunch is always free, and everything smells like red
roses at dawn, you shouldn't have to emit a BARRIER when the compiler
is in cfglayout mode.

Instead, fixup_reorder_chain *should* do it for you, but obviously
doesn't. There are several places where GCC code emits barriers while
in cfglayout mode, and that makes no sense because barriers are
meaningless in cfglayout mode :-)


> To trigger use the test in 59019 with an itanic cross compiler and comment
> out these two lines from gimple-ssa-isolate-paths.c:
>
>
>       TREE_THIS_VOLATILE (op) = 1;
>       TREE_SIDE_EFFECTS (op) = 1;

OK, thanks. Let's see if I can tackle this one.

Ciao!
Steven