ipa-profile.c: reset call_sums state within ipa-profile.c (v2; PR 93315)

Message ID 20200121135640.17844-1-dmalcolm@redhat.com
State New
Headers show
Series
  • ipa-profile.c: reset call_sums state within ipa-profile.c (v2; PR 93315)
Related show

Commit Message

David Malcolm Jan. 21, 2020, 1:56 p.m.
On Sat, 2020-01-18 at 18:42 +0100, Jan Hubicka wrote:
> > > > On Mon, 2020-01-13 at 11:23 +0800, luoxhu wrote:

> > > > On 2020/1/10 19:08, Jan Hubicka wrote:

> > > > > OK. You will need to do the obvious updates for Martin's

> > > > > patch

> > > > > which turned some member functions into static functions.

> > > > > 

> > > > > Honza

> > > > 

> > > > Thanks a lot!  Rebased & updated, will commit below patch

> > > > shortly

> > > > when git push is ready.

> > > > 

> > > > 

> > > > v8:

> > > >  1. Rebase to master with Martin's static function (r280043)

> > > > comments

> > > > merge.

> > > >     Boostrap/testsuite/SPEC2017 tested pass on Power8-LE.

> > > >  2. TODO:

> > > >     2.1. C++ devirt for multiple speculative call targets.

> > > >     2.2. ipa-icf ipa_merge_profiles refine with COMDAT inline

> > > > testcase.

> > > 

> > > [...]

> > > 

> > > > diff --git a/gcc/ipa-profile.c b/gcc/ipa-profile.c

> > > 

> > > [...]

> > > 

> > > > +static ipa_profile_call_summaries *call_sums = NULL;

> > > 

> > > [...]

> > >  

> > > >  static void

> > > >  ipa_profile_generate_summary (void)

> > > > @@ -169,7 +261,10 @@ ipa_profile_generate_summary (void)

> > > >    basic_block bb;

> > > >  

> > > >    hash_table<histogram_hash> hashtable (10);

> > > > -  

> > > > +

> > > > +  gcc_checking_assert (!call_sums);

> > > > +  call_sums = new ipa_profile_call_summaries (symtab);

> > > > +

> > > 

> > > [...]

> > > 

> > > Unfortunately, this assertion is failing for most of the

> > > testcases in

> > > jit.dg, reducing the number of PASS results in jit.sum from 10473

> > > down

> > > to 3254 in my builds.

> > > 

> > > 

> > > Counter-intuitively, "jit" is not in --enable-languages=all (as

> > > it also

> > > needs --enable-host-shared at configure time, which slows down

> > > the

> > > built compiler code).

> > > 

> > > 

> > > The jit code expects to be able to invoke the compiler code more

> > > than

> > > once within the same process, purging all state.

> > > 

> > > It looks like this "call_sums" state needs deleting and resetting

> > > to

> > > NULL after the compiler has run (or else we'll likely get an ICE

> > > due to

> > > using old symtab/call summaries in subsequent in-process runs of

> > > the

> > > compiler).

> > > 

> > > Is there a natural place to do that within the IPA hooks?  

> > > 

> > > 

> > > Alternatively the following patch seems to fix things (not yet

> > > fully

> > > tested though); hopefully it seems sane.

> > 

> > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu;

> > fixes

> > the issue with jit.sum.

> > 

> > OK for master?

> > 

> > gcc/ChangeLog:

> > 	PR ipa/93315

> > 	* ipa-profile.c (ipa_profile_c_finalize): New function.

> > 	* toplev.c (toplev::finalize): Call it.

> > 	* toplev.h (ipa_profile_c_finalize): New decl.

> 

> Other similar summaries are freed at the end of execute method.  I

> think

> that we probably want to do the same for consistency as well.

> Advantage is that this releases memory prior late

> compilation/streaming.


Thanks; here's an updated, much simpler patch, which does it at the
end of ipa_profile (which is effectively the execute method).

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu;
fixes the issue with jit.sum:

Changes to jit.sum
------------------

  FAIL: 69->0 (-69)
  PASS: 3254->10471 (+7217)
  UNRESOLVED: 1->0 (-1)

OK for master?

> I think for all these summaries we have leak for -flto compilation

> where

> we do not call execute methods and thus we do not free the summaries.

> Is this problem for jit?

> 

> Honza


If we do, then, if I understand correctly, this would only affect
someone who tried to use libgccjit to generate .o files with -flto,
repeatedly, within a single process.  I don't know of anyone doing
that, and if that's broken, that would be a separate, pre-existing,
bug, I think.

Dave

gcc/ChangeLog:
	PR ipa/93315
	* ipa-profile.c (ipa_profile): Delete call_sums and set it to
	NULL on exit.
---
 gcc/ipa-profile.c | 3 +++
 1 file changed, 3 insertions(+)

-- 
2.21.0

Comments

Jan Hubicka Jan. 21, 2020, 3:10 p.m. | #1
> 

> If we do, then, if I understand correctly, this would only affect

> someone who tried to use libgccjit to generate .o files with -flto,

> repeatedly, within a single process.  I don't know of anyone doing

> that, and if that's broken, that would be a separate, pre-existing,

> bug, I think.


Yes, i think we can play with that incrementally especially if someone
tries to use -flto with JIT setup (which by itself looks like bit of
overkill but perhaps things like offloading or so could make this
meaningful).

Honza
> 

> Dave

> 

> gcc/ChangeLog:

> 	PR ipa/93315

> 	* ipa-profile.c (ipa_profile): Delete call_sums and set it to

> 	NULL on exit.

> ---

>  gcc/ipa-profile.c | 3 +++

>  1 file changed, 3 insertions(+)

> 

> diff --git a/gcc/ipa-profile.c b/gcc/ipa-profile.c

> index 03272f20987..a69ba0c373a 100644

> --- a/gcc/ipa-profile.c

> +++ b/gcc/ipa-profile.c

> @@ -1023,6 +1023,9 @@ ipa_profile (void)

>    if (dump_file && (dump_flags & TDF_DETAILS))

>      symtab->dump (dump_file);

>  

> +  delete call_sums;

> +  call_sums = NULL;

> +

>    return 0;

>  }

>  

> -- 

> 2.21.0

>
David Malcolm Jan. 21, 2020, 3:32 p.m. | #2
On Tue, 2020-01-21 at 16:10 +0100, Jan Hubicka wrote:
> > If we do, then, if I understand correctly, this would only affect

> > someone who tried to use libgccjit to generate .o files with -flto,

> > repeatedly, within a single process.  I don't know of anyone doing

> > that, and if that's broken, that would be a separate, pre-existing,

> > bug, I think.

> 

> Yes, i think we can play with that incrementally especially if

> someone

> tries to use -flto with JIT setup (which by itself looks like bit of

> overkill but perhaps things like offloading or so could make this

> meaningful).

> 

> Honza

> > Dave


Thanks.  Is the patch OK?

> > gcc/ChangeLog:

> > 	PR ipa/93315

> > 	* ipa-profile.c (ipa_profile): Delete call_sums and set it to

> > 	NULL on exit.

> > ---

> >  gcc/ipa-profile.c | 3 +++

> >  1 file changed, 3 insertions(+)

> > 

> > diff --git a/gcc/ipa-profile.c b/gcc/ipa-profile.c

> > index 03272f20987..a69ba0c373a 100644

> > --- a/gcc/ipa-profile.c

> > +++ b/gcc/ipa-profile.c

> > @@ -1023,6 +1023,9 @@ ipa_profile (void)

> >    if (dump_file && (dump_flags & TDF_DETAILS))

> >      symtab->dump (dump_file);

> >  

> > +  delete call_sums;

> > +  call_sums = NULL;

> > +

> >    return 0;

> >  }

> >  

> > -- 

> > 2.21.0

> >
Jan Hubicka Jan. 21, 2020, 3:34 p.m. | #3
> On Tue, 2020-01-21 at 16:10 +0100, Jan Hubicka wrote:

> > > If we do, then, if I understand correctly, this would only affect

> > > someone who tried to use libgccjit to generate .o files with -flto,

> > > repeatedly, within a single process.  I don't know of anyone doing

> > > that, and if that's broken, that would be a separate, pre-existing,

> > > bug, I think.

> > 

> > Yes, i think we can play with that incrementally especially if

> > someone

> > tries to use -flto with JIT setup (which by itself looks like bit of

> > overkill but perhaps things like offloading or so could make this

> > meaningful).

> > 

> > Honza

> > > Dave

> 

> Thanks.  Is the patch OK?

Yes, i meant to apporve it :)
Thanks for looking into this.

Honza
> 

> > > gcc/ChangeLog:

> > > 	PR ipa/93315

> > > 	* ipa-profile.c (ipa_profile): Delete call_sums and set it to

> > > 	NULL on exit.

> > > ---

> > >  gcc/ipa-profile.c | 3 +++

> > >  1 file changed, 3 insertions(+)

> > > 

> > > diff --git a/gcc/ipa-profile.c b/gcc/ipa-profile.c

> > > index 03272f20987..a69ba0c373a 100644

> > > --- a/gcc/ipa-profile.c

> > > +++ b/gcc/ipa-profile.c

> > > @@ -1023,6 +1023,9 @@ ipa_profile (void)

> > >    if (dump_file && (dump_flags & TDF_DETAILS))

> > >      symtab->dump (dump_file);

> > >  

> > > +  delete call_sums;

> > > +  call_sums = NULL;

> > > +

> > >    return 0;

> > >  }

> > >  

> > > -- 

> > > 2.21.0

> > > 

>

Patch

diff --git a/gcc/ipa-profile.c b/gcc/ipa-profile.c
index 03272f20987..a69ba0c373a 100644
--- a/gcc/ipa-profile.c
+++ b/gcc/ipa-profile.c
@@ -1023,6 +1023,9 @@  ipa_profile (void)
   if (dump_file && (dump_flags & TDF_DETAILS))
     symtab->dump (dump_file);
 
+  delete call_sums;
+  call_sums = NULL;
+
   return 0;
 }