On 12/24/13 13:19, Steven Bosscher wrote:
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.
I certainly don't think you're trying to cause a deadlock, I think it's
just in the nature of how many maintainers work. Once someone reviews a
patch, the other maintainers tend to step back and let the submitter and
reviewer iterate. That's not always the case, but it happens enough to
be noticeable, IMHO. And it's not necessarily bad. Anyway...
Note carefully that we're not merging blocks without edges between them.
That's a misunderstanding, most likely due to my initial messages on
this issue. I thought I explained things better in my most recent
message, I'll try again :-)
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...)
No. That's a symptom of the problem. We were both barking up the wrong
tree earlier, in large part I'm sure due to my early conclusions.
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.
Umm, no. Read that paragraph again. Perhaps the RTL will help too.
Given:
(note 5 1 67 2 [bb 2] NOTE_INSN_BASIC_BLOCK)
[ ... ]
(jump_insn 17 16 19 2 (set (pc)
(if_then_else (ne (reg:CC 100 cc)
(const_int 0 [0]))
(label_ref 25)
(pc))) j.c:14 236 {arm_cond_branch}
(expr_list:REG_DEAD (reg:CC 100 cc)
(int_list:REG_BR_PROB 5000 (nil)))
-> 25)
(note 19 17 18 3 [bb 3] NOTE_INSN_BASIC_BLOCK)
(note/s 18 19 20 3 ("lab2") NOTE_INSN_DELETED_LABEL 3)
(note/s 20 18 21 ("lab") NOTE_INSN_DELETED_LABEL 4)
(barrier 21 20 25)
[ ... ]
We call merge blocks to merge blocks #2 and #3. Note well that block #2
reaches block #3 (and block #4). Block #3 has no successors. There's
no reason I can see that we can not request those blocks be merged. And
if they are merged, we *must* remove the BARRIER after block #3. If we
do that properly (per my most recent patch), all the other issues just
go away.
I propose we just punt on optimizing this case for now.
I disagree. There is no good reason why we can't handle this in a
sensible manner 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.
This problem can (and should) be addressed independently of a review of
the __builtin_unreachable semantics.
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.
I still prefer my most recent patch.
jeff
