| 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) {
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? OK for trunk if bootstrap and reg-test on x86_64 succeeds? Thanks, - Tom