| Message ID | 20171013133833.GB16196@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Check that there are no missing probabilities | expand |
On Fri, Oct 13, 2017 at 03:38:33PM +0200, Jan Hubicka wrote: > Hi, > this patch enables check that no edge probabilities are missing. > > Honza > > * cfghooks.c (verify_flow_info): Check that edge probabilities are > set. This broke bootstrap on x86_64-linux with Ada (--enable-checking=yes,rtl,extra). From what I can see, decompose_multiword_subregs has: 1619 /* Split the block after insn. There will be a fallthru 1620 edge, which is OK so we keep it. We have to create the 1621 exception edges ourselves. */ 1622 fallthru = split_block (bb, insn); 1623 rtl_make_eh_edge (NULL, bb, BB_END (bb)); 1624 bb = fallthru->dest; 1625 insn = BB_HEAD (bb); and rtl_make_eh_edge calls 161 make_label_edge (edge_cache, src, label, 162 EDGE_ABNORMAL | EDGE_EH 163 | (CALL_P (insn) ? EDGE_ABNORMAL_CALL : 0)); No idea what should initialize the probabilities. Do probabilities make any sense at all for EH edges (or abnormal edges)? > > Index: cfghooks.c > =================================================================== > --- cfghooks.c (revision 253694) > +++ cfghooks.c (working copy) > @@ -160,6 +161,13 @@ verify_flow_info (void) > e->src->index, e->dest->index); > err = 1; > } > + if (profile_status_for_fn (cfun) >= PROFILE_GUESSED > + && !e->probability.initialized_p ()) > + { > + error ("Uninitialized probability of edge %i->%i", e->src->index, > + e->dest->index); > + err = 1; > + } > if (!e->probability.verify ()) > { > error ("verify_flow_info: Wrong probability of edge %i->%i", Jakub
> On Fri, Oct 13, 2017 at 03:38:33PM +0200, Jan Hubicka wrote: > > Hi, > > this patch enables check that no edge probabilities are missing. > > > > Honza > > > > * cfghooks.c (verify_flow_info): Check that edge probabilities are > > set. > > This broke bootstrap on x86_64-linux with Ada > (--enable-checking=yes,rtl,extra). > > >From what I can see, decompose_multiword_subregs has: > 1619 /* Split the block after insn. There will be a fallthru > 1620 edge, which is OK so we keep it. We have to create the > 1621 exception edges ourselves. */ > 1622 fallthru = split_block (bb, insn); > 1623 rtl_make_eh_edge (NULL, bb, BB_END (bb)); > 1624 bb = fallthru->dest; > 1625 insn = BB_HEAD (bb); > and rtl_make_eh_edge calls > 161 make_label_edge (edge_cache, src, label, > 162 EDGE_ABNORMAL | EDGE_EH > 163 | (CALL_P (insn) ? EDGE_ABNORMAL_CALL : 0)); > > No idea what should initialize the probabilities. Do probabilities make > any sense at all for EH edges (or abnormal edges)? For EH we should set it to profile_probability::zero () because we know it is unlikely path. I will take a look. Honza > > > > > Index: cfghooks.c > > =================================================================== > > --- cfghooks.c (revision 253694) > > +++ cfghooks.c (working copy) > > @@ -160,6 +161,13 @@ verify_flow_info (void) > > e->src->index, e->dest->index); > > err = 1; > > } > > + if (profile_status_for_fn (cfun) >= PROFILE_GUESSED > > + && !e->probability.initialized_p ()) > > + { > > + error ("Uninitialized probability of edge %i->%i", e->src->index, > > + e->dest->index); > > + err = 1; > > + } > > if (!e->probability.verify ()) > > { > > error ("verify_flow_info: Wrong probability of edge %i->%i", > > Jakub
On Fri, Oct 13, 2017 at 09:06:55PM +0200, Jan Hubicka wrote: > For EH we should set it to profile_probability::zero () because we know it is unlikely > path. I will take a look. With the --- gcc/cfghooks.c.jj 2017-10-13 18:27:12.000000000 +0200 +++ gcc/cfghooks.c 2017-10-13 19:15:11.444650533 +0200 @@ -162,6 +162,7 @@ verify_flow_info (void) err = 1; } if (profile_status_for_fn (cfun) >= PROFILE_GUESSED + && (e->flags & (EDGE_EH | EDGE_ABNORMAL | EDGE_FAKE)) == 0 && !e->probability.initialized_p ()) { error ("Uninitialized probability of edge %i->%i", e->src->index, hack x86_64-linux and i686-linux bootstrapped fine, but I see still many graphite related regressions: /home/jakub/src/gcc/gcc/testsuite/gcc.dg/graphite/id-16.c:15:1: error: Uninitialized probability of edge 41->17 /home/jakub/src/gcc/gcc/testsuite/gcc.dg/graphite/id-16.c:15:1: error: Uninitialized probability of edge 44->41 /home/jakub/src/gcc/gcc/testsuite/gcc.dg/graphite/id-16.c:15:1: error: Uninitialized probability of edge 36->21 /home/jakub/src/gcc/gcc/testsuite/gcc.dg/graphite/id-16.c:15:1: error: Uninitialized probability of edge 29->36 /home/jakub/src/gcc/gcc/testsuite/gcc.dg/graphite/id-16.c:15:1: error: Uninitialized probability of edge 32->29 during GIMPLE pass: graphite dump file: id-16.c.150t.graphite /home/jakub/src/gcc/gcc/testsuite/gcc.dg/graphite/id-16.c:15:1: internal compiler error: verify_flow_info failed 0xafac1a verify_flow_info() ../../gcc/cfghooks.c:268 0xf2a624 checking_verify_flow_info ../../gcc/cfghooks.h:198 0xf2a624 cleanup_tree_cfg_noloop ../../gcc/tree-cfgcleanup.c:901 0xf2a624 cleanup_tree_cfg() ../../gcc/tree-cfgcleanup.c:952 0x162df85 graphite_transform_loops() ../../gcc/graphite.c:422 0x162f0c0 graphite_transforms ../../gcc/graphite.c:447 0x162f0c0 execute ../../gcc/graphite.c:524 So probably graphite needs to be tweaked for this too. Jakub
> On Fri, Oct 13, 2017 at 09:06:55PM +0200, Jan Hubicka wrote: > > For EH we should set it to profile_probability::zero () because we know it is unlikely > > path. I will take a look. > > With the > > --- gcc/cfghooks.c.jj 2017-10-13 18:27:12.000000000 +0200 > +++ gcc/cfghooks.c 2017-10-13 19:15:11.444650533 +0200 > @@ -162,6 +162,7 @@ verify_flow_info (void) > err = 1; > } > if (profile_status_for_fn (cfun) >= PROFILE_GUESSED > + && (e->flags & (EDGE_EH | EDGE_ABNORMAL | EDGE_FAKE)) == 0 > && !e->probability.initialized_p ()) > { > error ("Uninitialized probability of edge %i->%i", e->src->index, We should set probability of those edges to profile_probability::zero so we do not need to special case them at all places we check for profile. I fixed many occurences of bugs here but I see there are more. I will disable the check for now and take a look incrementally next week. Honza > > hack x86_64-linux and i686-linux bootstrapped fine, but I see still many > graphite related regressions: > > /home/jakub/src/gcc/gcc/testsuite/gcc.dg/graphite/id-16.c:15:1: error: Uninitialized probability of edge 41->17 > /home/jakub/src/gcc/gcc/testsuite/gcc.dg/graphite/id-16.c:15:1: error: Uninitialized probability of edge 44->41 > /home/jakub/src/gcc/gcc/testsuite/gcc.dg/graphite/id-16.c:15:1: error: Uninitialized probability of edge 36->21 > /home/jakub/src/gcc/gcc/testsuite/gcc.dg/graphite/id-16.c:15:1: error: Uninitialized probability of edge 29->36 > /home/jakub/src/gcc/gcc/testsuite/gcc.dg/graphite/id-16.c:15:1: error: Uninitialized probability of edge 32->29 > during GIMPLE pass: graphite > dump file: id-16.c.150t.graphite > /home/jakub/src/gcc/gcc/testsuite/gcc.dg/graphite/id-16.c:15:1: internal compiler error: verify_flow_info failed > 0xafac1a verify_flow_info() > ../../gcc/cfghooks.c:268 > 0xf2a624 checking_verify_flow_info > ../../gcc/cfghooks.h:198 > 0xf2a624 cleanup_tree_cfg_noloop > ../../gcc/tree-cfgcleanup.c:901 > 0xf2a624 cleanup_tree_cfg() > ../../gcc/tree-cfgcleanup.c:952 > 0x162df85 graphite_transform_loops() > ../../gcc/graphite.c:422 > 0x162f0c0 graphite_transforms > ../../gcc/graphite.c:447 > 0x162f0c0 execute > ../../gcc/graphite.c:524 > > So probably graphite needs to be tweaked for this too. > > Jakub
On Fri, Oct 13, 2017 at 6:38 AM, Jan Hubicka <hubicka@ucw.cz> wrote: > Hi, > this patch enables check that no edge probabilities are missing. This caused a bootstrap failure on aarch64-linux-gnu with go enabled. But I see you have disabled the code for now. Just for reference the failure: ../../../gcc/libgo/go/unicode/letter.go ../../../gcc/libgo/go/unicode/tables.go -o unicode.o >/dev/null 2>&1 ../../../gcc/libgo/go/runtime/panic.go: In function ‘runtime.gopanic’: ../../../gcc/libgo/go/runtime/panic.go:408:1: error: Uninitialized probability of edge 103->128 func gopanic(e interface{}) { ^ during RTL pass: subreg1 ../../../gcc/libgo/go/runtime/panic.go:408:1: internal compiler error: verify_flow_info failed 0x71f3b7 verify_flow_info() ../../gcc/gcc/cfghooks.c:267 0xa9402b execute_function_todo ../../gcc/gcc/passes.c:2006 0xa94de3 execute_todo ../../gcc/gcc/passes.c:2048 Please submit a full bug report, with preprocessed source if appropriate. Please include the complete backtrace with any bug report. See <https://gcc.gnu.org/bugs/> for instructions. Thanks, Andrew > > Honza > > * cfghooks.c (verify_flow_info): Check that edge probabilities are > set. > > Index: cfghooks.c > =================================================================== > --- cfghooks.c (revision 253694) > +++ cfghooks.c (working copy) > @@ -160,6 +161,13 @@ verify_flow_info (void) > e->src->index, e->dest->index); > err = 1; > } > + if (profile_status_for_fn (cfun) >= PROFILE_GUESSED > + && !e->probability.initialized_p ()) > + { > + error ("Uninitialized probability of edge %i->%i", e->src->index, > + e->dest->index); > + err = 1; > + } > if (!e->probability.verify ()) > { > error ("verify_flow_info: Wrong probability of edge %i->%i",
On Fri, Oct 13, 2017 at 9:27 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Oct 13, 2017 at 09:06:55PM +0200, Jan Hubicka wrote: >> For EH we should set it to profile_probability::zero () because we know it is unlikely >> path. I will take a look. > > With the > > --- gcc/cfghooks.c.jj 2017-10-13 18:27:12.000000000 +0200 > +++ gcc/cfghooks.c 2017-10-13 19:15:11.444650533 +0200 > @@ -162,6 +162,7 @@ verify_flow_info (void) > err = 1; > } > if (profile_status_for_fn (cfun) >= PROFILE_GUESSED > + && (e->flags & (EDGE_EH | EDGE_ABNORMAL | EDGE_FAKE)) == 0 > && !e->probability.initialized_p ()) > { > error ("Uninitialized probability of edge %i->%i", e->src->index, > > hack x86_64-linux and i686-linux bootstrapped fine, but I see still many > graphite related regressions: > > /home/jakub/src/gcc/gcc/testsuite/gcc.dg/graphite/id-16.c:15:1: error: Uninitialized probability of edge 41->17 > /home/jakub/src/gcc/gcc/testsuite/gcc.dg/graphite/id-16.c:15:1: error: Uninitialized probability of edge 44->41 > /home/jakub/src/gcc/gcc/testsuite/gcc.dg/graphite/id-16.c:15:1: error: Uninitialized probability of edge 36->21 > /home/jakub/src/gcc/gcc/testsuite/gcc.dg/graphite/id-16.c:15:1: error: Uninitialized probability of edge 29->36 > /home/jakub/src/gcc/gcc/testsuite/gcc.dg/graphite/id-16.c:15:1: error: Uninitialized probability of edge 32->29 > during GIMPLE pass: graphite > dump file: id-16.c.150t.graphite > /home/jakub/src/gcc/gcc/testsuite/gcc.dg/graphite/id-16.c:15:1: internal compiler error: verify_flow_info failed > 0xafac1a verify_flow_info() > ../../gcc/cfghooks.c:268 > 0xf2a624 checking_verify_flow_info > ../../gcc/cfghooks.h:198 > 0xf2a624 cleanup_tree_cfg_noloop > ../../gcc/tree-cfgcleanup.c:901 > 0xf2a624 cleanup_tree_cfg() > ../../gcc/tree-cfgcleanup.c:952 > 0x162df85 graphite_transform_loops() > ../../gcc/graphite.c:422 > 0x162f0c0 graphite_transforms > ../../gcc/graphite.c:447 > 0x162f0c0 execute > ../../gcc/graphite.c:524 > > So probably graphite needs to be tweaked for this too. graphite does if (changed) { cleanup_tree_cfg (); profile_status_for_fn (cfun) = PROFILE_ABSENT; release_recorded_exits (cfun); tree_estimate_probability (false); so it runs into CFG cleanup running before it properly resets counts. I wonder if we shouldn't simply get rid of the explicit checking calls in cfg cleanup... or if the profile checking should happen somewhere else. I'd also appreciate a better way for doing the above. Shouldn't we end up with a proper initialization on all edges as we just split existing ones and use create_empty_if_region_on_edge and create_empty_loop_on_edge? Ah, those use make_edge as well. The tree_estimate_probablility call above should be ideally replaced with sth like "propagate-SESE-entry-probability". Richard. > Jakub
> > graphite does > > if (changed) > { > cleanup_tree_cfg (); > profile_status_for_fn (cfun) = PROFILE_ABSENT; > release_recorded_exits (cfun); > tree_estimate_probability (false); > > so it runs into CFG cleanup running before it properly resets counts. > > I wonder if we shouldn't simply get rid of the explicit checking calls in > cfg cleanup... or if the profile checking should happen somewhere > else. > > I'd also appreciate a better way for doing the above. Shouldn't we > end up with a proper initialization on all edges as we just split > existing ones and use create_empty_if_region_on_edge and > create_empty_loop_on_edge? > > Ah, those use make_edge as well. > > The tree_estimate_probablility call above should be ideally > replaced with sth like "propagate-SESE-entry-probability". Well, re-running tree_estimate_probability is a hack and it won't really get you very sane profile update. I gues create_empty_if_region_on_edge and create_empty_loop_on_edge should care about profile, i will try to take a look. We have frequency propagation across SEME regions as part of find_sub_basic_blocks. It does not handle loops sanely though (which could be added), but still someone needs to care about correct probabilities at least. Honza > > Richard. > > > Jakub
Index: cfghooks.c =================================================================== --- cfghooks.c (revision 253694) +++ cfghooks.c (working copy) @@ -160,6 +161,13 @@ verify_flow_info (void) e->src->index, e->dest->index); err = 1; } + if (profile_status_for_fn (cfun) >= PROFILE_GUESSED + && !e->probability.initialized_p ()) + { + error ("Uninitialized probability of edge %i->%i", e->src->index, + e->dest->index); + err = 1; + } if (!e->probability.verify ()) { error ("verify_flow_info: Wrong probability of edge %i->%i",