check_cfg assert fix

Submitted by Tom de Vries on Sept. 6, 2011, 9:14 a.m.

Details

Message ID 4E65E463.6070004@codesourcery.com
State New
Headers show

Commit Message

Tom de Vries Sept. 6, 2011, 9:14 a.m.
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))));
...

It triggered on this rtl, a conditional return followed by a barrier:
...
(jump_insn 44 43 93 7 (set (pc)
	(if_then_else (ne (reg:CC 24 cc)
                (const_int 0 [0]))
            (return)
            (pc))) gcc/testsuite/gcc.dg/builtin-unreachable-4.c:13 249
{*cond_return}
     (expr_list:REG_DEAD (reg:CC 24 cc)
        (expr_list:REG_BR_PROB (const_int 9996 [0x270c])
            (nil)))
 -> return)

(barrier 93 44 92)
...

Although this insn sequence is non-optimal (the conditional return can be
optimized to an unconditional one, given that it's followed by a barrier), it's
not incorrect. The patch fixes this ICE by removing the check for the
'EDGE_COUNT (bb->succs) == 1' case.

Bootstrapped and reg-tested on x86_64 and build and reg-tested on arm.

OK for trunk?

Thanks,
- Tom

2011-09-05  Tom de Vries  <tom@codesourcery.com>

	* haifa-sched.c (check_cfg): Remove restriction on conditional jump if
	the containing block has only 1 outgoing edge.

Comments

Steven Bosscher Sept. 6, 2011, 3:13 p.m.
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))));
> ...
>
> It triggered on this rtl, a conditional return followed by a barrier:
> ...
> (jump_insn 44 43 93 7 (set (pc)
>        (if_then_else (ne (reg:CC 24 cc)
>                (const_int 0 [0]))
>            (return)
>            (pc))) gcc/testsuite/gcc.dg/builtin-unreachable-4.c:13 249
> {*cond_return}
>     (expr_list:REG_DEAD (reg:CC 24 cc)
>        (expr_list:REG_BR_PROB (const_int 9996 [0x270c])
>            (nil)))
>  -> return)
>
> (barrier 93 44 92)
> ...
>
> Although this insn sequence is non-optimal (the conditional return can be
> optimized to an unconditional one, given that it's followed by a barrier), it's
> not incorrect. The patch fixes this ICE by removing the check for the
> 'EDGE_COUNT (bb->succs) == 1' case.
>
> 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?

Ciao!
Steven
Maxim Kuvyrkov Sept. 6, 2011, 6 p.m.
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.

For reference, the test case in question:

/* { dg-do compile } */

void
foo ()
{
  asm goto (""::::l1);
  __builtin_unreachable ();
l1:;
}

void
bar ()
{
  foo ();
  foo ();
}

--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics

Patch hide | download patch | download mbox

Index: gcc/haifa-sched.c
===================================================================
--- gcc/haifa-sched.c (revision 178145)
+++ gcc/haifa-sched.c (working copy)
@@ -6065,13 +6065,12 @@  check_cfg (rtx head, rtx tail)
 		    gcc_assert (EDGE_COUNT (bb->succs) == 1
 				&& BARRIER_P (NEXT_INSN (head)));
 		  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))));
+		    gcc_assert (/* Weird case, like jump to the next insn
+				   or use of __builtin_unreachable ().  */
+				EDGE_COUNT (bb->succs) == 1
+				/* Normal case, one path falls through.  */
+				|| !BARRIER_P (NEXT_INSN (head)));
+
 		}
 	      if (BB_END (bb) == head)
 		{