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

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(+)

> 
> 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
>

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
> >

> 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
> > > 
>