| Message ID | 78c4bb49-712b-59ea-20f5-f7c39a569ab2@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Fix set of even probabilities (PR middle-end/89737). | expand |
On 3/19/19 2:02 AM, Martin Liška wrote: > Hi. > > When calling set_even_probabilities, the function assumes that an edge > can't live in both sets ({un,}likely_edges). When such situation happens, > clear just the sets. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? > Thanks, > Martin > > gcc/ChangeLog: > > 2019-03-18 Martin Liska <mliska@suse.cz> > > PR middle-end/89737 > * predict.c (combine_predictions_for_bb): Empty likely_edges and > unlikely_edges if there's an edge that belongs to both these sets. > > gcc/testsuite/ChangeLog: > > 2019-03-18 Martin Liska <mliska@suse.cz> > > PR middle-end/89737 > * gcc.dg/pr89737.c: New test. But is having the edge in both sets a valid state? You know this code better than I, so I'll go with your recommendation here. jeff
> On 3/19/19 2:02 AM, Martin Liška wrote: > > Hi. > > > > When calling set_even_probabilities, the function assumes that an edge > > can't live in both sets ({un,}likely_edges). When such situation happens, > > clear just the sets. > > > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > > > Ready to be installed? > > Thanks, > > Martin > > > > gcc/ChangeLog: > > > > 2019-03-18 Martin Liska <mliska@suse.cz> > > > > PR middle-end/89737 > > * predict.c (combine_predictions_for_bb): Empty likely_edges and > > unlikely_edges if there's an edge that belongs to both these sets. > > > > gcc/testsuite/ChangeLog: > > > > 2019-03-18 Martin Liska <mliska@suse.cz> > > > > PR middle-end/89737 > > * gcc.dg/pr89737.c: New test. > But is having the edge in both sets a valid state? You know this code > better than I, so I'll go with your recommendation here. It is the situation you add both hot and cold attribute which I guess can happen. I think it is OK to ignore both hint then, so patch is OK. Honza > > jeff
On Tue, Mar 19, 2019 at 09:02:27AM +0100, Martin Liška wrote: > 2019-03-18 Martin Liska <mliska@suse.cz> > > PR middle-end/89737 > * predict.c (combine_predictions_for_bb): Empty likely_edges and > unlikely_edges if there's an edge that belongs to both these sets. I admit I don't know this code at all, just wonder if if you find an intersection between likely_edges and unlikely_edges, couldn't you just remove the intersection from both, rather than all other edges too? Jakub
On 3/19/19 3:52 PM, Jakub Jelinek wrote: > On Tue, Mar 19, 2019 at 09:02:27AM +0100, Martin Liška wrote: >> 2019-03-18 Martin Liska <mliska@suse.cz> >> >> PR middle-end/89737 >> * predict.c (combine_predictions_for_bb): Empty likely_edges and >> unlikely_edges if there's an edge that belongs to both these sets. > > I admit I don't know this code at all, just wonder if if you find > an intersection between likely_edges and unlikely_edges, couldn't you just > remove the intersection from both, rather than all other edges too? Having a contradict edge makes any assumption about the likeliness of the others misleading. Plus it's very rare case this happens. I'm going to install the patch. Thanks, Martin > > Jakub >
diff --git a/gcc/predict.c b/gcc/predict.c index 43ee91a5b13..60a19d7edd1 100644 --- a/gcc/predict.c +++ b/gcc/predict.c @@ -1229,12 +1229,23 @@ combine_predictions_for_bb (basic_block bb, bool dry_run) if (pred->ep_probability <= PROB_VERY_UNLIKELY || pred->ep_predictor == PRED_COLD_LABEL) unlikely_edges.add (pred->ep_edge); - if (pred->ep_probability >= PROB_VERY_LIKELY - || pred->ep_predictor == PRED_BUILTIN_EXPECT - || pred->ep_predictor == PRED_HOT_LABEL) + else if (pred->ep_probability >= PROB_VERY_LIKELY + || pred->ep_predictor == PRED_BUILTIN_EXPECT + || pred->ep_predictor == PRED_HOT_LABEL) likely_edges.add (pred); } + /* It can happen that an edge is both in likely_edges and unlikely_edges. + Clear both sets in that situation. */ + for (hash_set<edge_prediction *>::iterator it = likely_edges.begin (); + it != likely_edges.end (); ++it) + if (unlikely_edges.contains ((*it)->ep_edge)) + { + likely_edges.empty (); + unlikely_edges.empty (); + break; + } + if (!dry_run) set_even_probabilities (bb, &unlikely_edges, &likely_edges); clear_bb_predictions (bb); diff --git a/gcc/testsuite/gcc.dg/pr89737.c b/gcc/testsuite/gcc.dg/pr89737.c new file mode 100644 index 00000000000..cd3dc81769e --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr89737.c @@ -0,0 +1,17 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-profile_estimate" } */ + +int a, b; + +void c() { + &&d; + void *e = &&f, *g = &&h; +f: + __attribute__((hot)) h : __attribute__((cold)) for (; a;) goto *g; +d: + for (; b;) + goto *e; +} + +/* { dg-final { scan-tree-dump-times "predicted to even probabilities" 4 "profile_estimate"} } */ +
Hi. When calling set_even_probabilities, the function assumes that an edge can't live in both sets ({un,}likely_edges). When such situation happens, clear just the sets. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin gcc/ChangeLog: 2019-03-18 Martin Liska <mliska@suse.cz> PR middle-end/89737 * predict.c (combine_predictions_for_bb): Empty likely_edges and unlikely_edges if there's an edge that belongs to both these sets. gcc/testsuite/ChangeLog: 2019-03-18 Martin Liska <mliska@suse.cz> PR middle-end/89737 * gcc.dg/pr89737.c: New test. --- gcc/predict.c | 17 ++++++++++++++--- gcc/testsuite/gcc.dg/pr89737.c | 17 +++++++++++++++++ 2 files changed, 31 insertions(+), 3 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/pr89737.c