Hi,
I am sorry, I am apparently not really able to follow all email this
week and am mostly skimming through this thread too, but...
On Thu, Feb 02, 2017 at 01:48:26PM +0100, Jan Hubicka wrote:
> >
> > 2017-02-02 Kugan Vivekanandarajah <kug...@linaro.org>
> >
> > * ipa-cp.c (ipcp_store_bits_results): Construct bits vector.
> > (ipcp_store_vr_results): Constrict m_vr vector.
> > * ipa-prop.c (ipa_node_params_t::remove): Set transaction summary to
> > null.
> > (ipa_node_params_t::duplicate): Construct bits and m_vr vector.
> > (read_ipcp_transformation_info): Likewise.
> >
> >
> > Thanks,
> > Kugan
> >
> > >>I tried similar think without variable structure like:
> > >>diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
> > >>index 93a2390c..b0cc832 100644
> > >>--- a/gcc/ipa-prop.h
> > >>+++ b/gcc/ipa-prop.h
> > >>@@ -525,7 +525,7 @@ struct GTY(()) ipcp_transformation_summary
> > >> /* Known bits information. */
> > >> vec<ipa_bits, va_gc> *bits;
> > >> /* Value range information. */
> > >>- vec<ipa_vr, va_gc> *m_vr;
> > >>+ vec<ipa_vr *, va_gc> *m_vr;
> > >> };
> > >>
> > >>This also has the same issue so I don't think it has anything to do with
> > >>variable structure.
> > >
> > >You have to debug that detail yourself but I wonder why the transformation
> > >summary has a different representation than the jump function (and I think
> > >the jump function size is the issue).
> > >
> > >The JF has
> > >
> > > /* Information about zero/non-zero bits. */
> > > struct ipa_bits bits;
> > >
> > > /* Information about value range, containing valid data only when
> > > vr_known is
> > > true. */
> > > value_range m_vr;
> > > bool vr_known;
> > >
> > >with ipa_bits having two widest_ints and value_range having two trees
> > >and an unused bitmap and ipa_vr having two wide_ints (widest_ints are
> > >smaller!).
> > >
> > >Richard.
> > >
> > >>
> > >>Thanks,
> > >>Kugan
>
> > diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> > index aa3c997..5103555 100644
> > --- a/gcc/ipa-cp.c
> > +++ b/gcc/ipa-cp.c
> > @@ -4865,6 +4865,8 @@ ipcp_store_bits_results (void)
> >
> > ipcp_grow_transformations_if_necessary ();
> > ipcp_transformation_summary *ts = ipcp_get_transformation_summary
> > (node);
> > + if (!ts->bits)
> > + ts->bits = (new (ggc_cleared_alloc<ipa_bits_vec> ()) ipa_bits_vec ());
> > vec_safe_reserve_exact (ts->bits, count);
> >
> > for (unsigned i = 0; i < count; i++)
> > @@ -4940,6 +4942,8 @@ ipcp_store_vr_results (void)
> >
> > ipcp_grow_transformations_if_necessary ();
> > ipcp_transformation_summary *ts = ipcp_get_transformation_summary
> > (node);
> > + if (!ts->m_vr)
> > + ts->m_vr = new (ggc_cleared_alloc<ipa_vr_vec> ()) ipa_vr_vec ();
> > vec_safe_reserve_exact (ts->m_vr, count);
> >
> > for (unsigned i = 0; i < count; i++)
> > diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
> > index 834c27d..b992a7f 100644
> > --- a/gcc/ipa-prop.c
> > +++ b/gcc/ipa-prop.c
> > @@ -3759,13 +3759,20 @@ ipa_node_params_t::insert (cgraph_node *,
> > ipa_node_params *info)
> > to. */
> >
> > void
> > -ipa_node_params_t::remove (cgraph_node *, ipa_node_params *info)
> > +ipa_node_params_t::remove (cgraph_node *node, ipa_node_params *info)
> > {
> > free (info->lattices);
> > /* Lattice values and their sources are deallocated with their alocation
> > pool. */
> > info->known_csts.release ();
> > info->known_contexts.release ();
> > + ipcp_transformation_summary *ts = ipcp_get_transformation_summary (node);
> > + if (ts != NULL)
why des this need to be conditional? ipcp_get_transformation_summary
also lives in garbage collector so it should be able to hold any
necessary references properly.
> > + {
> > + ts->agg_values = NULL;
> > + ts->bits = NULL;
> > + ts->m_vr = NULL;
> > + }
>
> I would go for explicit ggc_free for them: garbage collrector is hardly ever
> run during
> WPA stage and thus we are not going to get the memory back otherwise.
ggc_freeing might make a difference but I fail to see how the above
can, unless ipa_node_params_t still holds a reference to the info,
which it is about to drop. Moreover...
>
> Patch is OK with that change.
I believe this patch conflicts with Martin's fix for 79337 which might
deal with parts of what Kugan wants to achieve better so it may be
better to re-base the patch?
Thanks,
Martin
