Message ID | 20200129114659.GN83181@kam.mff.cuni.cz |
---|---|
State | New |
Headers | show |
Series | ipa: Fix removal of multi-target speculation | expand |
On 1/29/20 12:46 PM, Jan Hubicka wrote: > Hi, > this fixes indirect call speculation ICE Martin (Liska) reduced from > spec GCC. The problem here was in resolve_speculation that if called to > resolve the second edge in multi-target speculative call resolved first > on instead. This is bug I did not catch during the API change. > Other problem is that redirect_call_stmt_to_callee if called from > inliner is called on direct part of speculative call while it expects > indirect. This is checking only failure but rahter than removing sanity > check I switch to indirect edge. > > profiled-lto-bootstrapped x86_64-linux and comitted. Thanks for the patch ... > > Honza > > 2020-01-29 Jan Hubicka <hubicka@ucw.cz> > > * cgraph.c (cgraph_edge::resolve_speculation): Only lookup direct edge > if called on indirect edge. > (cgraph_edge::redirect_call_stmt_to_callee): Lookup indirect edge of > speculative call if needed. > > gcc/testsuite/ChangeLog: > > 2020-01-29 Jan Hubicka <hubicka@ucw.cz> > > * gcc.dg/tree-prof/indir-call-prof-2.c: New testcase. > > > diff --git a/gcc/cgraph.c b/gcc/cgraph.c > index 3e50b0bc380..294b2d392fd 100644 > --- a/gcc/cgraph.c > +++ b/gcc/cgraph.c > @@ -1189,7 +1189,10 @@ cgraph_edge::resolve_speculation (cgraph_edge *edge, tree callee_decl) > ipa_ref *ref; > > gcc_assert (edge->speculative && (!callee_decl || edge->callee)); > - e2 = edge->first_speculative_call_target (); > + if (!edge->callee) > + e2 = edge->first_speculative_call_target (); > + else > + e2 = edge; > ref = e2->speculative_call_target_ref (); > edge = edge->speculative_call_indirect_edge (); > if (!callee_decl > @@ -1364,7 +1367,8 @@ cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e) > /* If there already is an direct call (i.e. as a result of inliner's > substitution), forget about speculating. */ > if (decl) > - e = make_direct (e, cgraph_node::get (decl)); > + e = make_direct (e->speculative_call_indirect_edge (), > + cgraph_node::get (decl)); > else > { > /* Be sure we redirect all speculative targets before poking > diff --git a/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c b/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c > index 61612b5b628..bbba0521018 100644 > --- a/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c > +++ b/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c This should be a new file. > @@ -1,25 +1,28 @@ > /* { dg-options "-O2 -fno-early-inlining -fdump-ipa-profile-optimized -fdump-ipa-afdo" } */ > volatile int one; > -static > -int add1 (int val) > +static int > +add1 (int val) > { > - return val+=one; > + return val += one; > } > -static > -int sub1 (int val) > + > +static int > +sub1 (int val) > { > - return val-=one; > + return val -= one; > } > -static > -int do_op (int val, int (*fnptr) (int)) > + > +static int > +do_op (int val, int (*fnptr) (int)) > { > return fnptr (val); > } > + > int > -main(void) > +main (void) > { > - int i,val; > - for (i=0;i<100000;i++) > + int i, val = 0; > + for (i = 0; i < 100000; i++) > { > val = do_op (val, add1); > val = do_op (val, sub1); > diff --git a/libgcc/libgcov-merge.c b/libgcc/libgcov-merge.c ... and this should not be installed. Martin > index 19b8ee72ae9..801037d0d64 100644 > --- a/libgcc/libgcov-merge.c > +++ b/libgcc/libgcov-merge.c > @@ -115,7 +115,7 @@ merge_topn_values_set (gcov_type *counters) > continue; > > unsigned j; > - int slot = -1; > + int slot = 0; > > for (j = 0; j < GCOV_TOPN_VALUES; j++) > { > @@ -124,24 +124,15 @@ merge_topn_values_set (gcov_type *counters) > counters[2 * j + 1] += read_counters[2 * i + 1]; > break; > } > - else if (counters[2 * j + 1] == 0) > + if (counters[2 * j + 1] < counters[2 * slot + 1]) > slot = j; > } > > - if (j == GCOV_TOPN_VALUES) > + if (j == GCOV_TOPN_VALUES > + && counters[2 * slot + 1] < read_counters[2 * i + 1]) > { > - if (slot > 0) > - { > - /* If we found empty slot, add the value. */ > - counters[2 * slot] = read_counters[2 * i]; > - counters[2 * slot + 1] = read_counters[2 * i + 1]; > - } > - else > - { > - /* We haven't found a slot, bail out. */ > - counters[1] = -1; > - return; > - } > + counters[2 * slot] = read_counters[2 * i]; > + counters[2 * slot + 1] = read_counters[2 * i + 1]; > } > } > } >
> > diff --git a/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c b/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c > > index 61612b5b628..bbba0521018 100644 > > --- a/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c > > +++ b/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c > > This should be a new file. Sorry, git is still not my friend. I am used to add new files when I star them, then fix them and then expect them as new files in diff. git insists on diffing over the original file I added. > > > @@ -1,25 +1,28 @@ > > /* { dg-options "-O2 -fno-early-inlining -fdump-ipa-profile-optimized -fdump-ipa-afdo" } */ > > volatile int one; > > -static > > -int add1 (int val) > > +static int > > +add1 (int val) > > { > > - return val+=one; > > + return val += one; > > } > > -static > > -int sub1 (int val) > > + > > +static int > > +sub1 (int val) > > { > > - return val-=one; > > + return val -= one; > > } > > -static > > -int do_op (int val, int (*fnptr) (int)) > > + > > +static int > > +do_op (int val, int (*fnptr) (int)) > > { > > return fnptr (val); > > } > > + > > int > > -main(void) > > +main (void) > > { > > - int i,val; > > - for (i=0;i<100000;i++) > > + int i, val = 0; > > + for (i = 0; i < 100000; i++) > > { > > val = do_op (val, add1); > > val = do_op (val, sub1); > > diff --git a/libgcc/libgcov-merge.c b/libgcc/libgcov-merge.c > > ... and this should not be installed. .. and this was accidentally included in patch (it is hack to disable reproducibility). Thanks for pointing this out. Honza
On 1/29/20 1:39 PM, Jan Hubicka wrote: >>> diff --git a/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c b/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c >>> index 61612b5b628..bbba0521018 100644 >>> --- a/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c >>> +++ b/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c >> >> This should be a new file. > Sorry, git is still not my friend. I am used to add new files when I > star them, then fix them and then expect them as new files in diff. Then it's a diff against a file in git index. Then you need to do something like: $ git diff HEAD > git insists on diffing over the original file I added. Anyway, I would definitely recommend to make a commit and then send a result with git format-patch -1 HEAD. That's more safe than a diff. Martin >> >>> @@ -1,25 +1,28 @@ >>> /* { dg-options "-O2 -fno-early-inlining -fdump-ipa-profile-optimized -fdump-ipa-afdo" } */ >>> volatile int one; >>> -static >>> -int add1 (int val) >>> +static int >>> +add1 (int val) >>> { >>> - return val+=one; >>> + return val += one; >>> } >>> -static >>> -int sub1 (int val) >>> + >>> +static int >>> +sub1 (int val) >>> { >>> - return val-=one; >>> + return val -= one; >>> } >>> -static >>> -int do_op (int val, int (*fnptr) (int)) >>> + >>> +static int >>> +do_op (int val, int (*fnptr) (int)) >>> { >>> return fnptr (val); >>> } >>> + >>> int >>> -main(void) >>> +main (void) >>> { >>> - int i,val; >>> - for (i=0;i<100000;i++) >>> + int i, val = 0; >>> + for (i = 0; i < 100000; i++) >>> { >>> val = do_op (val, add1); >>> val = do_op (val, sub1); >>> diff --git a/libgcc/libgcov-merge.c b/libgcc/libgcov-merge.c >> >> ... and this should not be installed. > > .. and this was accidentally included in patch (it is hack to disable > reproducibility). > Thanks for pointing this out. > > Honza >
diff --git a/gcc/cgraph.c b/gcc/cgraph.c index 3e50b0bc380..294b2d392fd 100644 --- a/gcc/cgraph.c +++ b/gcc/cgraph.c @@ -1189,7 +1189,10 @@ cgraph_edge::resolve_speculation (cgraph_edge *edge, tree callee_decl) ipa_ref *ref; gcc_assert (edge->speculative && (!callee_decl || edge->callee)); - e2 = edge->first_speculative_call_target (); + if (!edge->callee) + e2 = edge->first_speculative_call_target (); + else + e2 = edge; ref = e2->speculative_call_target_ref (); edge = edge->speculative_call_indirect_edge (); if (!callee_decl @@ -1364,7 +1367,8 @@ cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge *e) /* If there already is an direct call (i.e. as a result of inliner's substitution), forget about speculating. */ if (decl) - e = make_direct (e, cgraph_node::get (decl)); + e = make_direct (e->speculative_call_indirect_edge (), + cgraph_node::get (decl)); else { /* Be sure we redirect all speculative targets before poking diff --git a/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c b/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c index 61612b5b628..bbba0521018 100644 --- a/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c +++ b/gcc/testsuite/gcc.dg/tree-prof/indir-call-prof-2.c @@ -1,25 +1,28 @@ /* { dg-options "-O2 -fno-early-inlining -fdump-ipa-profile-optimized -fdump-ipa-afdo" } */ volatile int one; -static -int add1 (int val) +static int +add1 (int val) { - return val+=one; + return val += one; } -static -int sub1 (int val) + +static int +sub1 (int val) { - return val-=one; + return val -= one; } -static -int do_op (int val, int (*fnptr) (int)) + +static int +do_op (int val, int (*fnptr) (int)) { return fnptr (val); } + int -main(void) +main (void) { - int i,val; - for (i=0;i<100000;i++) + int i, val = 0; + for (i = 0; i < 100000; i++) { val = do_op (val, add1); val = do_op (val, sub1); diff --git a/libgcc/libgcov-merge.c b/libgcc/libgcov-merge.c index 19b8ee72ae9..801037d0d64 100644 --- a/libgcc/libgcov-merge.c +++ b/libgcc/libgcov-merge.c @@ -115,7 +115,7 @@ merge_topn_values_set (gcov_type *counters) continue; unsigned j; - int slot = -1; + int slot = 0; for (j = 0; j < GCOV_TOPN_VALUES; j++) { @@ -124,24 +124,15 @@ merge_topn_values_set (gcov_type *counters) counters[2 * j + 1] += read_counters[2 * i + 1]; break; } - else if (counters[2 * j + 1] == 0) + if (counters[2 * j + 1] < counters[2 * slot + 1]) slot = j; } - if (j == GCOV_TOPN_VALUES) + if (j == GCOV_TOPN_VALUES + && counters[2 * slot + 1] < read_counters[2 * i + 1]) { - if (slot > 0) - { - /* If we found empty slot, add the value. */ - counters[2 * slot] = read_counters[2 * i]; - counters[2 * slot + 1] = read_counters[2 * i + 1]; - } - else - { - /* We haven't found a slot, bail out. */ - counters[1] = -1; - return; - } + counters[2 * slot] = read_counters[2 * i]; + counters[2 * slot + 1] = read_counters[2 * i + 1]; } } }