Re: Check that there are no missing probabilities
>
> 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
Re: Check that there are no missing probabilities
On Fri, Oct 13, 2017 at 9:27 PM, Jakub Jelinek 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.0 +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
Re: Check that there are no missing probabilities
On Fri, Oct 13, 2017 at 6:38 AM, Jan Hubicka 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
Re: Check that there are no missing probabilities
> 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.0 +0200
> +++ gcc/cfghooks.c2017-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
Re: Check that there are no missing probabilities
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.0 +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
Re: Check that there are no missing probabilities
> 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. */
> 1622fallthru = split_block (bb, insn);
> 1623rtl_make_eh_edge (NULL, bb, BB_END (bb));
> 1624bb = fallthru->dest;
> 1625insn = 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
Re: Check that there are no missing probabilities
This patch also caused a huge number of testsuite failures on PowerPC, although it didn't break bootstrap. Thanks, David
Re: Check that there are no missing probabilities
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,
162EDGE_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
Check that there are no missing probabilities
Hi,
this patch enables check that no edge probabilities are missing.
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",
