Re: [PATCH] Verify edge probability consistency in verify_flow_info

[Jeff Law <law at redhat dot com>]

On 08/02/2017 10:07 AM, Tom de Vries wrote:
> Hi,
> 
> I.
> 
> for target nvptx we recently ran into PR81442, an ICE in verify_flow_info:
> ...
> error: verify_flow_info: REG_BR_PROB is set but cfg probability is not
> ...
> 
> We start out with a jump instruction:
> ...
> (jump_insn 18 17 31 2 (set (pc)
>         (if_then_else (ne (reg:BI 83)
>                 (const_int 0 [0]))
>             (label_ref 31)
>             (pc))) "reduction-5.c":52 104 {br_true}
>      (expr_list:REG_DEAD (reg:BI 83)
>         (int_list:REG_BR_PROB 10000 (nil)))
>  -> 31)
> ...
> 
> The probability is set on the branch edge, but not on the fallthru edge.
> 
> After fixup_reorder_chain, the condition is reversed, and the
> probability reg-note update accordingly:
> ...
> (jump_insn 18 17 32 2 (set (pc)
>         (if_then_else (eq (reg:BI 83)
>                 (const_int 0 [0]))
>             (label_ref:DI 33)
>             (pc))) "reduction-5.c":52 105 {br_false}
>      (expr_list:REG_DEAD (reg:BI 83)
>         (int_list:REG_BR_PROB 0 (nil)))
>  -> 33)
> ...
> 
> The branch and fallthru edge are also reversed, which means now the
> probability is set on the fallthru edge, and not on the branch edge.
> 
> This is the immediate cause for the verify_flow_info error.
> 
> 
> II.
> 
> We've fixed the ICE in the target by setting the probability on the
> fallthru edge when we introduce it (r250422).
> 
> We've noted other places where we were forgetting to set the probability
> (fixed in r250823), but that did not trigger the ICE.
> 
> 
> III.
> 
> I've written this patch to check for the missing probability more
> consistently. I'm not certain if we can require that the probability
> should always be set, so I'm just requiring that if it is set on one
> outgoing edge, it needs to be set on all outgoing edges.
> 
> Sofar I've build a c-only x86_64 non-bootstrap compiler and ran dg.exp.
> The only problem I ran into was in attr-simd{,-2,-4}.c. I've written a
> tentative patch for that, and will submit it as follow-up.
> 
> Is this check a good idea?
I think the additional checking is a good idea.  Ideally we'd verify
that all edges have a probability.  Until then I think you need some
kind of rationale in a comment for why the checking is limited.

> 
> OK for trunk if bootstrap and reg-test on x86_64 succeeds?
Yea, but I'd like to see ongoing work towards full checking.

Jeff