On Fri, Dec 20, 2013 at 7:30 PM, Jeff Law wrote:
>
> So here's an alternate approach to fixing 59285. I still think attacking
> this in rtl_merge_blocks is better, but with nobody else chiming in to break
> the deadlock Steven and myself are in, I'll go with Steven's preferred
> solution (fix the callers in ifcvt.c).
I didn't intend to cause a deadlock, I only really want us to respect
the rules of the CFG, one of which is that you can't merge two basic
blocks that are not connected by an edge. I think this is a really
important invariant because it avoids accidental basic block merges
that are not correct.
> If we were to return to a "fix rtl_merge_blocks" approach, I would revamp
> that patch to utilize the ideas in this one. Namely that it's not just
> barriers between the merged blocks that are a problem. In fact, that's a
> symptom of the problem. Things have already gone wrong by that point.
What has gone wrong at that point, is that we'd be trying to merge two
basic blocks that have no control flow connection. The case of
builtin_unreachable (the only legitimate case for an empty basic block
without successors) is a special case. (This is the reason why I would
like us to have a special instruction or some kind of other marker for
builtin_unreachable...)
> Given blocks A & B that will be merged. If A has > 1 successor and B has no
> successors, the combined block will always have at least 1 successor.
> However, the combined block will be followed by a BARRIER that must be
> removed.
Note this would happen automatically if there as an edge connecting
the blocks and a JUMP_INSN ending block B.
I propose we just punt on optimizing this case for now. For GCC 4.10
we should define what behavior should result from builtin_unreachable
(Should it trap? Can it be optimized away after a while to avoid these
unnecessary conditional jumps? ...) but for the moment it seems wrong
IMHO to only optimize this in the cond_exec case and to do so against
the rules of the control flow graph.
Something like the patch below, tested with a cross-compiler for arm-eabi.
What do you think of this approach?
PR middle-end/59285
* ifcvt. (cond_exec_find_if_block): Do not try to if-convert an empty
basic block without successors due to builtin_unreachable.
Index: ifcvt.c
===================================================================
--- ifcvt.c (revision 206195)
+++ ifcvt.c (working copy)
@@ -3495,6 +3495,13 @@ cond_exec_find_if_block (struct ce_if_block * ce_i
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. */
+ /* To make things worse: A block that ended in builtin_unreachable is
+ usually empty. Perhaps we should optimize these away, but the semantics
+ of builtin_unreachable are not really clear about this, and if we do
+ optimize builtin_unreachable here (i.e. in the cond_exec path) we have
+ a strange difference of semantics of builtin_unreachable on cond_exec
+ and non-cond_exec targets. Therefore, at least for now, don't merge
+ away a builtin_unreachable block. */
if (EDGE_COUNT (then_bb->succs) == 0)
{
if (single_pred_p (else_bb) && else_bb != EXIT_BLOCK_PTR_FOR_FN (cfun))
@@ -3511,6 +3518,12 @@ cond_exec_find_if_block (struct ce_if_block * ce_i
&& ! simplejump_p (last_insn))
return FALSE;
+ /* Empty block (no non-note insns anyway) only happens with
+ builtin_unreachable. merge_if_blocks isn't prepared for
+ that. See PR59285. */
+ if (last_insn == BB_HEAD (then_bb))
+ return FALSE;
+
join_bb = else_bb;
else_bb = NULL_BLOCK;
}
