check_cfg assert fix

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:



Ciao!
Steven

On Tue, Sep 6, 2011 at 11:31 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> 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)
>                {
>

BTW that's one ugly gcc_assert. Candidate for gcc_checking_assert?

Ciao!
Steven

On 7/09/2011, at 9:32 AM, Steven Bosscher wrote:

> On Tue, Sep 6, 2011 at 11:31 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
>> 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)
>>                {
>> 
> 
> BTW that's one ugly gcc_assert. Candidate for gcc_checking_assert?

I agree.  I would rather remove the entirety of haifa-sched.c: check_cfg(); scheduler is not the right place for checking consistency of CFG.  Check_cfg() was useful for debugging scheduler patches, but now it is more of maintainance overhead.

Do I have a second vote for removal of check_cfg()?

--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics

On 09/06/11 23:56, Maxim Kuvyrkov wrote:
> I agree.  I would rather remove the entirety of haifa-sched.c:
> check_cfg(); scheduler is not the right place for checking
> consistency of CFG.  Check_cfg() was useful for debugging scheduler
> patches, but now it is more of maintainance overhead.
> 
> Do I have a second vote for removal of check_cfg()?

I'd be OK with that. Saves me some time adapting it to some scheduler
patches I'll be submitting soon...


Bernd

On 9/09/2011, at 6:54 AM, Bernd Schmidt wrote:

> On 09/06/11 23:56, Maxim Kuvyrkov wrote:
>> I agree.  I would rather remove the entirety of haifa-sched.c:
>> check_cfg(); scheduler is not the right place for checking
>> consistency of CFG.  Check_cfg() was useful for debugging scheduler
>> patches, but now it is more of maintainance overhead.
>> 
>> Do I have a second vote for removal of check_cfg()?
> 
> I'd be OK with that. Saves me some time adapting it to some scheduler
> patches I'll be submitting soon...

OK then, attached is the trivial patch that removes haifa-sched.c:check_cfg().  Please let me know if you have strong feelings towards keeping check_cfg().

Tested on x86_64-linux-gnu.  Absent any requests to the contrary, I will check in this patch in 2 days.

Thank you,

--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics

On 14/09/2011, at 7:40 AM, Maxim Kuvyrkov wrote:
> 
> OK then, attached is the trivial patch that removes haifa-sched.c:check_cfg().  Please let me know if you have strong feelings towards keeping check_cfg().
> 
> Tested on x86_64-linux-gnu.  Absent any requests to the contrary, I will check in this patch in 2 days.

Checked in.

--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics