[PATCH] check_cfg assert fix

[Steven Bosscher]

On Tue, Sep 6, 2011 at 8:00 PM, Maxim Kuvyrkov <maxim@codesourcery.com> wrote:
>
> On 7/09/2011, at 3:13 AM, Steven Bosscher wrote:
>
>> On Tue, Sep 6, 2011 at 11:14 AM, Tom de Vries <vries@codesourcery.com> wrote:
>>> Hi,
>>>
>>> During testing the approved-for-commit middle-end patch for bug 43864 on ARM, I
>>> ran into a gcc.dg/torture/pr46068.c ICE.
>>>
>>> The following assert in haifa-sched.c:check_cfg triggered:
>>> ...
>>>                  else if (any_condjump_p (head))
>>>                    gcc_assert (/* Usual case.  */
>>>                                (EDGE_COUNT (bb->succs) > 1
>>>                                 && !BARRIER_P (NEXT_INSN (head)))
>>>                                /* Or jump to the next instruction.  */
>>>                                || (EDGE_COUNT (bb->succs) == 1
>>>                                    && (BB_HEAD (EDGE_I (bb->succs, 0)->dest)
>>>                                        == JUMP_LABEL (head))));
>>>
> ...
>>> Bootstrapped and reg-tested on x86_64 and build and reg-tested on arm.
>>>
>>> OK for trunk?
>>
>> No. If the conditional return is not taken, you should not fall into a
>> barrier. It looks like the CFG somehow got corrupted, and if that's
>> indeed the case then your patch just papers over the problem. That
>> follows after the barrier?
>
> Initially, I thought so too, that is why I wrote the above assert in the first place.  However, __builtin_unreachable() adds a whole new spin on where a BARRIER can occur; after all, this built-in expands directly to BARRIER.
>
> Before Tom's optimization the fall-through path of conditional jump was followed by an unconditional jump to a label directly followed by a barrier.  Tom's patch removed the middle-man in the face of unconditional jump so that the barrier now follows a conditional jump.  This is very odd, but, given the initial test case, is a valid case.

Well, shouldn't the assert be changed then to handle !onlyjump_p and
returnjump_p cases separately? Tom's patch removes a valid check for
all but these corner cases. Perhaps something like this:

Index: haifa-sched.c
===================================================================
--- haifa-sched.c	(revision 178601)
+++ haifa-sched.c	(working copy)
@@ -6071,7 +6071,10 @@ check_cfg (rtx head, rtx tail)
                                 /* Or jump to the next instruction.  */
                                 || (EDGE_COUNT (bb->succs) == 1
                                     && (BB_HEAD (EDGE_I (bb->succs, 0)->dest)
-                                        == JUMP_LABEL (head))));
+                                        == JUMP_LABEL (head)))
+				/* Or the jump is not just a jump.  */
+				|| (!onlyjump_p (head)
+				    || returnjump_p (head)));
 		}
 	      if (BB_END (bb) == head)
 		{


Ciao!
Steven