Message ID | 61e0af9b-205c-644a-b1ee-42e035d61cc1@mentor.com |
---|---|
State | New |
Headers | show |
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
> > > > 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. I have full checking in my tree for some time. At x86-64 bootstrap there is one remaining offender in simd_clone_adjust which was not fixed yet https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00219.html Jakub did not tell me what would be a reasonable guess :) After that I plan to enable full checking after checking arm/ppc. So I hope we will converge to full checking really soon. But having additional check is fine. Honza > > Jeff
On 08/04/2017 11:15 AM, Jan Hubicka wrote: >>> OK for trunk if bootstrap and reg-test on x86_64 succeeds? >> Yea, but I'd like to see ongoing work towards full checking. > > I have full checking in my tree for some time. At x86-64 bootstrap there > is one remaining offender in simd_clone_adjust which was not fixed yet > https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00219.html > Jakub did not tell me what would be a reasonable guess :) I think I just fixed that one here ( https://gcc.gnu.org/ml/gcc-cvs/2017-08/msg00112.html ). Thanks, - Tom
> On 08/04/2017 11:15 AM, Jan Hubicka wrote: > >>>OK for trunk if bootstrap and reg-test on x86_64 succeeds? > >>Yea, but I'd like to see ongoing work towards full checking. > > > >I have full checking in my tree for some time. At x86-64 bootstrap there > >is one remaining offender in simd_clone_adjust which was not fixed yet > >https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00219.html > >Jakub did not tell me what would be a reasonable guess :) > > I think I just fixed that one here ( > https://gcc.gnu.org/ml/gcc-cvs/2017-08/msg00112.html ). Thanks! I will re-check then where we are standing wrt enabling checking everywhere ;) Honza > > Thanks, > - Tom
Verify edge probability consistency in verify_flow_info 2017-08-02 Tom de Vries <tom@codesourcery.com> * cfghooks.c (verify_flow_info): Verify that if one outgoing edge has probability set, all outgoing edges have probability set. --- gcc/cfghooks.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/gcc/cfghooks.c b/gcc/cfghooks.c index 18dc49a..b973651 100644 --- a/gcc/cfghooks.c +++ b/gcc/cfghooks.c @@ -152,6 +152,8 @@ verify_flow_info (void) bb->index, bb->frequency); err = 1; } + bool has_prob_uninit_edges = false; + bool has_prob_init_edges = false; FOR_EACH_EDGE (e, ei, bb->succs) { if (last_visited [e->dest->index] == bb) @@ -166,6 +168,10 @@ verify_flow_info (void) e->src->index, e->dest->index); err = 1; } + if (e->probability.initialized_p ()) + has_prob_init_edges = true; + else + has_prob_uninit_edges = true; if (!e->count.verify ()) { error ("verify_flow_info: Wrong count of edge %i->%i", @@ -197,6 +203,12 @@ verify_flow_info (void) error ("wrong amount of branch edges after unconditional jump %i", bb->index); err = 1; } + if (has_prob_uninit_edges && has_prob_init_edges) + { + error ("Missing edge probability after unconditional jump in bb %i", + bb->index); + err = 1; + } FOR_EACH_EDGE (e, ei, bb->preds) {