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

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

Commit Message

David Malcolm Jan. 17, 2020, 10:32 p.m.
> > 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.
---
 gcc/ipa-profile.c | 10 ++++++++++
 gcc/toplev.c      |  1 +
 gcc/toplev.h      |  2 ++
 3 files changed, 13 insertions(+)

-- 
2.21.0

Comments

Jan Hubicka Jan. 18, 2020, 5:42 p.m. | #1
> > > 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.

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

Patch

diff --git a/gcc/ipa-profile.c b/gcc/ipa-profile.c
index 03272f20987..5f7621b1432 100644
--- a/gcc/ipa-profile.c
+++ b/gcc/ipa-profile.c
@@ -1070,3 +1070,13 @@  make_pass_ipa_profile (gcc::context *ctxt)
 {
   return new pass_ipa_profile (ctxt);
 }
+
+/* Reset all state within ipa-profile.c so that we can rerun the compiler
+   within the same process.  For use by toplev::finalize.  */
+
+void
+ipa_profile_c_finalize ()
+{
+  delete call_sums;
+  call_sums = NULL;
+}
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 4c8be502c71..ca0515583c7 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -2453,6 +2453,7 @@  toplev::finalize (void)
   this_target_rtl->target_specific_initialized = false;
 
   /* Needs to be called before cgraph_c_finalize since it uses symtab.  */
+  ipa_profile_c_finalize ();
   ipa_reference_c_finalize ();
   ipa_fnsummary_c_finalize ();
 
diff --git a/gcc/toplev.h b/gcc/toplev.h
index d6c316962b0..10611691608 100644
--- a/gcc/toplev.h
+++ b/gcc/toplev.h
@@ -103,4 +103,6 @@  extern void parse_alignment_opts (void);
 
 extern void initialize_rtl (void);
 
+extern void ipa_profile_c_finalize (void);
+
 #endif /* ! GCC_TOPLEV_H */