check_cfg assert fix

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.

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

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