On Fri, Nov 18, 2016 at 09:09:40AM +0100, Richard Biener wrote:
> > + /* Make a copy of BB, merge it into PRED. */
> > + basic_block copy = duplicate_block (bb, e, NULL);
> > + emit_barrier_after_bb (copy);
> > + reorder_insns_nobb (BB_HEAD (copy), BB_END (copy), BB_END (pred));
> > + merge_blocks (pred, copy);
>
> there is can_merge_blocks_p and I would have expected the RTL CFG hooks
> to do the insn "merging" (you use reorder_insns_nobb for this which is
> actually deprecated?). I suppose if you'd specify pred as third arg
> to duplicate_block the CFG hook would have done its work
> (can_merge_blocks_p checks a->next_bb == b)? I'm not that familiar
> with CFG-RTL mode.
The third arg to duplicate_block ("after") doesn't do anything in either
RTL mode: it only is passed to move_block_after, but that is a noop for
the RTL modes (and its result is not checked by duplicate_block, ouch).
can_merge_blocks_p will always return true here (except I forgot to check
for simplejump_p -- I'll add that).
rtl_merge_blocks does not check rtl_can_merge_blocks_p (and you get a
quite spectacular ICE when you get it wrong: everything between the two
blocks is deleted :-) ). I'll make a separate patch that checks it
(layout mode already does).
reorder_insns_nobb is deprecated but it does an important job for which
there is no replacement. In many cases you can do better than doing
manual surgery on the insn stream of course.
Thanks for the review,
Segher
