| Message ID | c8477826-61d1-7b44-d3e3-364fa1a52588@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Jan 30, 2017 at 12:23 AM, kugan <kugan.vivekanandarajah@linaro.org> wrote: > Hi All, > > As suggested by Richard in the PR, I tried to implement variable size > structures for VR as shown in attached patch. That is, I changed ipa-prop.h > to: > > diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h > index 93a2390c..acab2aa 100644 > --- a/gcc/ipa-prop.h > +++ b/gcc/ipa-prop.h > @@ -157,13 +157,15 @@ struct GTY(()) ipa_bits > }; > > /* Info about value ranges. */ > -struct GTY(()) ipa_vr > +struct GTY ((variable_size)) ipa_vr > { > /* The data fields below are valid only if known is true. */ > bool known; > enum value_range_type type; > - wide_int min; > - wide_int max; > + /* Minimum and maximum. */ > + TRAILING_WIDE_INT_ACCESSOR (min, ints, 0) > + TRAILING_WIDE_INT_ACCESSOR (max, ints, 1) > + trailing_wide_ints <2> ints; > }; > > /* A jump function for a callsite represents the values passed as actual > @@ -525,7 +527,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; > }; > > void ipa_set_node_agg_value_chain (struct cgraph_node *node, > > However, I am running into error when I do LTO bootstrap that memory seems > to have deallocated by the garbage collector. Since we have the reference to > the memory allocated by ggc_internal_alloc in the vector (m_vr), I thought > it will not be deallocated. But during the bootstrap, when in > ipa_node_params_t::duplicate, it seems to have been deallocated as shown in > the back trace. I dont understand internals of gc in gcc so any help is > appreciated. > > > lto1: internal compiler error: Segmentation fault > 0xdedc4b crash_signal > ../../gcc/gcc/toplev.c:333 > 0xb46680 ipa_node_params_t::duplicate(cgraph_node*, cgraph_node*, > ipa_node_params*, ipa_node_params*) > ../../gcc/gcc/ipa-prop.c:3819 > 0xb306a3 > function_summary<ipa_node_params*>::symtab_duplication(cgraph_node*, > cgraph_node*, void*) > ../../gcc/gcc/symbol-summary.h:187 > 0x85aba7 symbol_table::call_cgraph_duplication_hooks(cgraph_node*, > cgraph_node*) > ../../gcc/gcc/cgraph.c:488 > 0x8765bf cgraph_node::create_clone(tree_node*, long, int, bool, > vec<cgraph_edge*, va_heap, vl_ptr>, bool, cgraph_node*, bitmap_head*, char > const*) > ../../gcc/gcc/cgraphclones.c:522 > 0x166fb3b clone_inlined_nodes(cgraph_edge*, bool, bool, int*, int) > ../../gcc/gcc/ipa-inline-transform.c:227 > 0x166fbd7 clone_inlined_nodes(cgraph_edge*, bool, bool, int*, int) > ../../gcc/gcc/ipa-inline-transform.c:242 > 0x1670893 inline_call(cgraph_edge*, bool, vec<cgraph_edge*, va_heap, > vl_ptr>*, int*, bool, bool*) > ../../gcc/gcc/ipa-inline-transform.c:449 > 0x1665bd3 inline_small_functions > ../../gcc/gcc/ipa-inline.c:2024 > 0x1667157 ipa_inline > ../../gcc/gcc/ipa-inline.c:2434 > 0x1667fa7 execute > ../../gcc/gcc/ipa-inline.c:2845 > > > 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
Hi Richard, On 30/01/17 21:08, Richard Biener wrote: > On Mon, Jan 30, 2017 at 12:23 AM, kugan > <kugan.vivekanandarajah@linaro.org> wrote: >> lto1: internal compiler error: Segmentation fault >> 0xdedc4b crash_signal >> ../../gcc/gcc/toplev.c:333 >> 0xb46680 ipa_node_params_t::duplicate(cgraph_node*, cgraph_node*, >> ipa_node_params*, ipa_node_params*) >> ../../gcc/gcc/ipa-prop.c:3819 >> 0xb306a3 >> function_summary<ipa_node_params*>::symtab_duplication(cgraph_node*, >> cgraph_node*, void*) >> ../../gcc/gcc/symbol-summary.h:187 >> 0x85aba7 symbol_table::call_cgraph_duplication_hooks(cgraph_node*, >> cgraph_node*) >> ../../gcc/gcc/cgraph.c:488 >> 0x8765bf cgraph_node::create_clone(tree_node*, long, int, bool, >> vec<cgraph_edge*, va_heap, vl_ptr>, bool, cgraph_node*, bitmap_head*, char >> const*) >> ../../gcc/gcc/cgraphclones.c:522 >> 0x166fb3b clone_inlined_nodes(cgraph_edge*, bool, bool, int*, int) >> ../../gcc/gcc/ipa-inline-transform.c:227 >> 0x166fbd7 clone_inlined_nodes(cgraph_edge*, bool, bool, int*, int) >> ../../gcc/gcc/ipa-inline-transform.c:242 >> 0x1670893 inline_call(cgraph_edge*, bool, vec<cgraph_edge*, va_heap, >> vl_ptr>*, int*, bool, bool*) >> ../../gcc/gcc/ipa-inline-transform.c:449 >> 0x1665bd3 inline_small_functions >> ../../gcc/gcc/ipa-inline.c:2024 >> 0x1667157 ipa_inline >> ../../gcc/gcc/ipa-inline.c:2434 >> 0x1667fa7 execute >> ../../gcc/gcc/ipa-inline.c:2845 >> This is due to an existing issue. That is, in ipa_node_params_t::remove, m_vr and bits vectors are not set to null such that the gc can claim it. I also noticed that we don't create m_vr and bits vectors. Attached patch does this. This was bootstrapped and regression tested with the above patch. I am now testing the attached patch alone. Is this OK if no regressions? gcc/ChangeLog: 2017-02-02 Kugan Vivekanandarajah <kuganv@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, >> Kugandiff --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) + { + ts->agg_values = NULL; + ts->bits = NULL; + ts->m_vr = NULL; + } } /* Hook that is called by summary when a node is duplicated. */ @@ -3811,8 +3818,10 @@ ipa_node_params_t::duplicate(cgraph_node *src, cgraph_node *dst, ipcp_grow_transformations_if_necessary (); src_trans = ipcp_get_transformation_summary (src); const vec<ipa_vr, va_gc> *src_vr = src_trans->m_vr; - vec<ipa_vr, va_gc> *&dst_vr - = ipcp_get_transformation_summary (dst)->m_vr; + ipcp_transformation_summary *dts = ipcp_get_transformation_summary (dst); + if (!dts->m_vr) + dts->m_vr = (new (ggc_cleared_alloc<ipa_vr_vec> ()) ipa_vr_vec ()); + vec<ipa_vr, va_gc> *&dst_vr = dts->m_vr; if (vec_safe_length (src_trans->m_vr) > 0) { vec_safe_reserve_exact (dst_vr, src_vr->length ()); @@ -3826,8 +3835,10 @@ ipa_node_params_t::duplicate(cgraph_node *src, cgraph_node *dst, ipcp_grow_transformations_if_necessary (); src_trans = ipcp_get_transformation_summary (src); const vec<ipa_bits, va_gc> *src_bits = src_trans->bits; - vec<ipa_bits, va_gc> *&dst_bits - = ipcp_get_transformation_summary (dst)->bits; + ipcp_transformation_summary *dts = ipcp_get_transformation_summary (dst); + if (!dts->bits) + dts->bits = (new (ggc_cleared_alloc<ipa_bits_vec> ()) ipa_bits_vec ()); + vec<ipa_bits, va_gc> *&dst_bits = dts->bits; vec_safe_reserve_exact (dst_bits, src_bits->length ()); for (unsigned i = 0; i < src_bits->length (); ++i) dst_bits->quick_push ((*src_bits)[i]); @@ -5283,6 +5294,8 @@ read_ipcp_transformation_info (lto_input_block *ib, cgraph_node *node, 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_grow_cleared (ts->m_vr, count); for (i = 0; i < count; i++) { @@ -5306,6 +5319,8 @@ read_ipcp_transformation_info (lto_input_block *ib, cgraph_node *node, 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_grow_cleared (ts->bits, count); for (i = 0; i < count; i++) diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h index 93a2390c..6573a78 100644 --- a/gcc/ipa-prop.h +++ b/gcc/ipa-prop.h @@ -528,6 +528,9 @@ struct GTY(()) ipcp_transformation_summary vec<ipa_vr, va_gc> *m_vr; }; +typedef vec<ipa_vr, va_gc> ipa_vr_vec; +typedef vec<ipa_bits, va_gc> ipa_bits_vec; + void ipa_set_node_agg_value_chain (struct cgraph_node *node, struct ipa_agg_replacement_value *aggvals); void ipcp_grow_transformations_if_necessary (void);
Hi Richard, On 30/01/17 21:08, Richard Biener wrote: > On Mon, Jan 30, 2017 at 12:23 AM, kugan > <kugan.vivekanandarajah@linaro.org> wrote: >> Hi All, >> >> As suggested by Richard in the PR, I tried to implement variable size >> structures for VR as shown in attached patch. That is, I changed ipa-prop.h >> to: >> >> diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h >> index 93a2390c..acab2aa 100644 >> --- a/gcc/ipa-prop.h >> +++ b/gcc/ipa-prop.h >> @@ -157,13 +157,15 @@ struct GTY(()) ipa_bits >> }; >> >> /* Info about value ranges. */ >> -struct GTY(()) ipa_vr >> +struct GTY ((variable_size)) ipa_vr >> { >> /* The data fields below are valid only if known is true. */ >> bool known; >> enum value_range_type type; >> - wide_int min; >> - wide_int max; >> + /* Minimum and maximum. */ >> + TRAILING_WIDE_INT_ACCESSOR (min, ints, 0) >> + TRAILING_WIDE_INT_ACCESSOR (max, ints, 1) >> + trailing_wide_ints <2> ints; >> }; >> >> /* A jump function for a callsite represents the values passed as actual >> @@ -525,7 +527,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; >> }; >> >> void ipa_set_node_agg_value_chain (struct cgraph_node *node, >> >> However, I am running into error when I do LTO bootstrap that memory seems >> to have deallocated by the garbage collector. Since we have the reference to >> the memory allocated by ggc_internal_alloc in the vector (m_vr), I thought >> it will not be deallocated. But during the bootstrap, when in >> ipa_node_params_t::duplicate, it seems to have been deallocated as shown in >> the back trace. I dont understand internals of gc in gcc so any help is >> appreciated. >> >> >> lto1: internal compiler error: Segmentation fault >> 0xdedc4b crash_signal >> ../../gcc/gcc/toplev.c:333 >> 0xb46680 ipa_node_params_t::duplicate(cgraph_node*, cgraph_node*, >> ipa_node_params*, ipa_node_params*) >> ../../gcc/gcc/ipa-prop.c:3819 >> 0xb306a3 >> function_summary<ipa_node_params*>::symtab_duplication(cgraph_node*, >> cgraph_node*, void*) >> ../../gcc/gcc/symbol-summary.h:187 >> 0x85aba7 symbol_table::call_cgraph_duplication_hooks(cgraph_node*, >> cgraph_node*) >> ../../gcc/gcc/cgraph.c:488 >> 0x8765bf cgraph_node::create_clone(tree_node*, long, int, bool, >> vec<cgraph_edge*, va_heap, vl_ptr>, bool, cgraph_node*, bitmap_head*, char >> const*) >> ../../gcc/gcc/cgraphclones.c:522 >> 0x166fb3b clone_inlined_nodes(cgraph_edge*, bool, bool, int*, int) >> ../../gcc/gcc/ipa-inline-transform.c:227 >> 0x166fbd7 clone_inlined_nodes(cgraph_edge*, bool, bool, int*, int) >> ../../gcc/gcc/ipa-inline-transform.c:242 >> 0x1670893 inline_call(cgraph_edge*, bool, vec<cgraph_edge*, va_heap, >> vl_ptr>*, int*, bool, bool*) >> ../../gcc/gcc/ipa-inline-transform.c:449 >> 0x1665bd3 inline_small_functions >> ../../gcc/gcc/ipa-inline.c:2024 >> 0x1667157 ipa_inline >> ../../gcc/gcc/ipa-inline.c:2434 >> 0x1667fa7 execute >> ../../gcc/gcc/ipa-inline.c:2845 >> >> >> 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!). > We now have ipa_vr (with wide_int) for ipcp_transaction_summary. value_range is used in jump functions as it uses tree-vrp for most of the handling. Attached patch uses variable_structure for ipa_vr. A version of this patch passed regression and lto bootstrapped (before I separated the part that prevented testing and posted that separately in https://gcc.gnu.org/ml/gcc-patches/2017-02/msg00103.html). Does this approach looks OK? I will do full testing and post again based on the feedback. I am also going to do the same for ipa-bits based on the feedback. Do you also want to do variable structure with widest_ints? Thanks, KuganFrom e2cd620b8876b67066fc2781f68adaa087094669 Mon Sep 17 00:00:00 2001 From: Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org> Date: Thu, 2 Feb 2017 13:37:27 +1100 Subject: [PATCH 2/2] p2 --- gcc/ipa-cp.c | 24 +++++++++++++------- gcc/ipa-prop.c | 69 ++++++++++++++++++++++++++++++++++++---------------------- gcc/ipa-prop.h | 12 +++++----- 3 files changed, 66 insertions(+), 39 deletions(-) diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c index 5103555..79c9894 100644 --- a/gcc/ipa-cp.c +++ b/gcc/ipa-cp.c @@ -4949,21 +4949,29 @@ ipcp_store_vr_results (void) for (unsigned i = 0; i < count; i++) { ipcp_param_lattices *plats = ipa_get_parm_lattices (info, i); - ipa_vr vr; + ipa_vr *vr; if (!plats->m_value_range.bottom_p () && !plats->m_value_range.top_p ()) { - vr.known = true; - vr.type = plats->m_value_range.m_vr.type; - vr.min = plats->m_value_range.m_vr.min; - vr.max = plats->m_value_range.m_vr.max; + wide_int min = plats->m_value_range.m_vr.min; + wide_int max = plats->m_value_range.m_vr.max; + unsigned int precision = min.get_precision (); + size_t size = (sizeof (ipa_vr) + + trailing_wide_ints <2>::extra_size (precision)); + vr = static_cast<ipa_vr *> (ggc_internal_alloc (size)); + vr->ints.set_precision (precision); + vr->known = true; + vr->type = plats->m_value_range.m_vr.type; + vr->set_min (min); + vr->set_max (max); } else { - vr.known = false; - vr.type = VR_VARYING; - vr.min = vr.max = wi::zero (INT_TYPE_SIZE); + size_t size = (sizeof (ipa_vr)); + vr = static_cast<ipa_vr *> (ggc_internal_alloc (size)); + vr->known = false; + vr->type = VR_VARYING; } ts->m_vr->quick_push (vr); } diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c index b992a7f..81f83b8 100644 --- a/gcc/ipa-prop.c +++ b/gcc/ipa-prop.c @@ -3817,11 +3817,11 @@ ipa_node_params_t::duplicate(cgraph_node *src, cgraph_node *dst, { ipcp_grow_transformations_if_necessary (); src_trans = ipcp_get_transformation_summary (src); - const vec<ipa_vr, va_gc> *src_vr = src_trans->m_vr; + const vec<ipa_vr *, va_gc> *src_vr = src_trans->m_vr; ipcp_transformation_summary *dts = ipcp_get_transformation_summary (dst); if (!dts->m_vr) dts->m_vr = (new (ggc_cleared_alloc<ipa_vr_vec> ()) ipa_vr_vec ()); - vec<ipa_vr, va_gc> *&dst_vr = dts->m_vr; + vec<ipa_vr *, va_gc> *&dst_vr = dts->m_vr; if (vec_safe_length (src_trans->m_vr) > 0) { vec_safe_reserve_exact (dst_vr, src_vr->length ()); @@ -5224,16 +5224,16 @@ write_ipcp_transformation_info (output_block *ob, cgraph_node *node) for (unsigned i = 0; i < count; ++i) { struct bitpack_d bp; - ipa_vr *parm_vr = &(*ts->m_vr)[i]; + ipa_vr *parm_vr = (*ts->m_vr)[i]; bp = bitpack_create (ob->main_stream); bp_pack_value (&bp, parm_vr->known, 1); streamer_write_bitpack (&bp); if (parm_vr->known) { - streamer_write_enum (ob->main_stream, value_rang_type, + streamer_write_enum (ob->main_stream, value_range_type, VR_LAST, parm_vr->type); - streamer_write_wide_int (ob, parm_vr->min); - streamer_write_wide_int (ob, parm_vr->max); + streamer_write_wide_int (ob, parm_vr->get_min ()); + streamer_write_wide_int (ob, parm_vr->get_max ()); } } } @@ -5296,21 +5296,38 @@ read_ipcp_transformation_info (lto_input_block *ib, cgraph_node *node, 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_grow_cleared (ts->m_vr, count); + vec_safe_reserve_exact (ts->m_vr, count); for (i = 0; i < count; i++) { ipa_vr *parm_vr; - parm_vr = &(*ts->m_vr)[i]; struct bitpack_d bp; bp = streamer_read_bitpack (ib); - parm_vr->known = bp_unpack_value (&bp, 1); - if (parm_vr->known) + bool known = bp_unpack_value (&bp, 1); + if (known) + { + enum value_range_type type = streamer_read_enum (ib, + value_range_type, + VR_LAST); + wide_int min = streamer_read_wide_int (ib); + wide_int max = streamer_read_wide_int (ib); + unsigned int precision = min.get_precision (); + size_t size = (sizeof (ipa_vr) + + trailing_wide_ints <2>::extra_size (precision)); + parm_vr = static_cast<ipa_vr *> (ggc_internal_alloc (size)); + parm_vr->ints.set_precision (precision); + parm_vr->known = known; + parm_vr->type = type; + parm_vr->set_min (min); + parm_vr->set_max (max); + } + else { - parm_vr->type = streamer_read_enum (ib, value_range_type, - VR_LAST); - parm_vr->min = streamer_read_wide_int (ib); - parm_vr->max = streamer_read_wide_int (ib); + size_t size = (sizeof (ipa_vr)); + parm_vr = static_cast<ipa_vr *> (ggc_internal_alloc (size)); + parm_vr->known = known; + parm_vr->type = VR_VARYING; } + ts->m_vr->quick_push (parm_vr); } } count = streamer_read_uhwi (ib); @@ -5688,7 +5705,7 @@ ipcp_update_vr (struct cgraph_node *node) ipcp_transformation_summary *ts = ipcp_get_transformation_summary (node); if (!ts || vec_safe_length (ts->m_vr) == 0) return; - const vec<ipa_vr, va_gc> &vr = *ts->m_vr; + const vec<ipa_vr *, va_gc> &vr = *ts->m_vr; unsigned count = vr.length (); for (unsigned i = 0; i < count; ++i, parm = next_parm) @@ -5703,8 +5720,8 @@ ipcp_update_vr (struct cgraph_node *node) if (!ddef || !is_gimple_reg (parm)) continue; - if (vr[i].known - && (vr[i].type == VR_RANGE || vr[i].type == VR_ANTI_RANGE)) + if (vr[i]->known + && (vr[i]->type == VR_RANGE || vr[i]->type == VR_ANTI_RANGE)) { tree type = TREE_TYPE (ddef); unsigned prec = TYPE_PRECISION (type); @@ -5714,22 +5731,22 @@ ipcp_update_vr (struct cgraph_node *node) { fprintf (dump_file, "Setting value range of param %u ", i); fprintf (dump_file, "%s[", - (vr[i].type == VR_ANTI_RANGE) ? "~" : ""); - print_decs (vr[i].min, dump_file); + (vr[i]->type == VR_ANTI_RANGE) ? "~" : ""); + print_decs (vr[i]->get_min (), dump_file); fprintf (dump_file, ", "); - print_decs (vr[i].max, dump_file); + print_decs (vr[i]->get_max (), dump_file); fprintf (dump_file, "]\n"); } - set_range_info (ddef, vr[i].type, - wide_int_storage::from (vr[i].min, prec, + set_range_info (ddef, vr[i]->type, + wide_int_storage::from (vr[i]->get_min (), prec, TYPE_SIGN (type)), - wide_int_storage::from (vr[i].max, prec, + wide_int_storage::from (vr[i]->get_max (), prec, TYPE_SIGN (type))); } else if (POINTER_TYPE_P (TREE_TYPE (ddef)) - && vr[i].type == VR_ANTI_RANGE - && wi::eq_p (vr[i].min, 0) - && wi::eq_p (vr[i].max, 0)) + && vr[i]->type == VR_ANTI_RANGE + && wi::eq_p (vr[i]->get_min (), 0) + && wi::eq_p (vr[i]->get_max (), 0)) { if (dump_file) fprintf (dump_file, "Setting nonnull for %u\n", i); diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h index 6573a78..ffbf007 100644 --- a/gcc/ipa-prop.h +++ b/gcc/ipa-prop.h @@ -157,13 +157,15 @@ struct GTY(()) ipa_bits }; /* Info about value ranges. */ -struct GTY(()) ipa_vr +struct GTY ((variable_size)) ipa_vr { /* The data fields below are valid only if known is true. */ bool known; enum value_range_type type; - wide_int min; - wide_int max; + /* Minimum and maximum. */ + TRAILING_WIDE_INT_ACCESSOR (min, ints, 0) + TRAILING_WIDE_INT_ACCESSOR (max, ints, 1) + trailing_wide_ints <2> ints; }; /* A jump function for a callsite represents the values passed as actual @@ -525,10 +527,10 @@ 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; }; -typedef vec<ipa_vr, va_gc> ipa_vr_vec; +typedef vec<ipa_vr *, va_gc> ipa_vr_vec; typedef vec<ipa_bits, va_gc> ipa_bits_vec; void ipa_set_node_agg_value_chain (struct cgraph_node *node, -- 2.7.4
> > 2017-02-02 Kugan Vivekanandarajah <kuganv@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) > + { > + 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. Patch is OK with that change. Honza > } > > /* Hook that is called by summary when a node is duplicated. */ > @@ -3811,8 +3818,10 @@ ipa_node_params_t::duplicate(cgraph_node *src, cgraph_node *dst, > ipcp_grow_transformations_if_necessary (); > src_trans = ipcp_get_transformation_summary (src); > const vec<ipa_vr, va_gc> *src_vr = src_trans->m_vr; > - vec<ipa_vr, va_gc> *&dst_vr > - = ipcp_get_transformation_summary (dst)->m_vr; > + ipcp_transformation_summary *dts = ipcp_get_transformation_summary (dst); > + if (!dts->m_vr) > + dts->m_vr = (new (ggc_cleared_alloc<ipa_vr_vec> ()) ipa_vr_vec ()); > + vec<ipa_vr, va_gc> *&dst_vr = dts->m_vr; > if (vec_safe_length (src_trans->m_vr) > 0) > { > vec_safe_reserve_exact (dst_vr, src_vr->length ()); > @@ -3826,8 +3835,10 @@ ipa_node_params_t::duplicate(cgraph_node *src, cgraph_node *dst, > ipcp_grow_transformations_if_necessary (); > src_trans = ipcp_get_transformation_summary (src); > const vec<ipa_bits, va_gc> *src_bits = src_trans->bits; > - vec<ipa_bits, va_gc> *&dst_bits > - = ipcp_get_transformation_summary (dst)->bits; > + ipcp_transformation_summary *dts = ipcp_get_transformation_summary (dst); > + if (!dts->bits) > + dts->bits = (new (ggc_cleared_alloc<ipa_bits_vec> ()) ipa_bits_vec ()); > + vec<ipa_bits, va_gc> *&dst_bits = dts->bits; > vec_safe_reserve_exact (dst_bits, src_bits->length ()); > for (unsigned i = 0; i < src_bits->length (); ++i) > dst_bits->quick_push ((*src_bits)[i]); > @@ -5283,6 +5294,8 @@ read_ipcp_transformation_info (lto_input_block *ib, cgraph_node *node, > 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_grow_cleared (ts->m_vr, count); > for (i = 0; i < count; i++) > { > @@ -5306,6 +5319,8 @@ read_ipcp_transformation_info (lto_input_block *ib, cgraph_node *node, > 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_grow_cleared (ts->bits, count); > > for (i = 0; i < count; i++) > diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h > index 93a2390c..6573a78 100644 > --- a/gcc/ipa-prop.h > +++ b/gcc/ipa-prop.h > @@ -528,6 +528,9 @@ struct GTY(()) ipcp_transformation_summary > vec<ipa_vr, va_gc> *m_vr; > }; > > +typedef vec<ipa_vr, va_gc> ipa_vr_vec; > +typedef vec<ipa_bits, va_gc> ipa_bits_vec; > + > void ipa_set_node_agg_value_chain (struct cgraph_node *node, > struct ipa_agg_replacement_value *aggvals); > void ipcp_grow_transformations_if_necessary (void);
On Thu, Feb 2, 2017 at 1:48 PM, Jan Hubicka <hubicka@ucw.cz> wrote: >> >> 2017-02-02 Kugan Vivekanandarajah <kuganv@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 ()); Why do you need those placement news? Richard. >> 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) >> + { >> + 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. > > Patch is OK with that change. > > Honza >> } >> >> /* Hook that is called by summary when a node is duplicated. */ >> @@ -3811,8 +3818,10 @@ ipa_node_params_t::duplicate(cgraph_node *src, cgraph_node *dst, >> ipcp_grow_transformations_if_necessary (); >> src_trans = ipcp_get_transformation_summary (src); >> const vec<ipa_vr, va_gc> *src_vr = src_trans->m_vr; >> - vec<ipa_vr, va_gc> *&dst_vr >> - = ipcp_get_transformation_summary (dst)->m_vr; >> + ipcp_transformation_summary *dts = ipcp_get_transformation_summary (dst); >> + if (!dts->m_vr) >> + dts->m_vr = (new (ggc_cleared_alloc<ipa_vr_vec> ()) ipa_vr_vec ()); >> + vec<ipa_vr, va_gc> *&dst_vr = dts->m_vr; >> if (vec_safe_length (src_trans->m_vr) > 0) >> { >> vec_safe_reserve_exact (dst_vr, src_vr->length ()); >> @@ -3826,8 +3835,10 @@ ipa_node_params_t::duplicate(cgraph_node *src, cgraph_node *dst, >> ipcp_grow_transformations_if_necessary (); >> src_trans = ipcp_get_transformation_summary (src); >> const vec<ipa_bits, va_gc> *src_bits = src_trans->bits; >> - vec<ipa_bits, va_gc> *&dst_bits >> - = ipcp_get_transformation_summary (dst)->bits; >> + ipcp_transformation_summary *dts = ipcp_get_transformation_summary (dst); >> + if (!dts->bits) >> + dts->bits = (new (ggc_cleared_alloc<ipa_bits_vec> ()) ipa_bits_vec ()); >> + vec<ipa_bits, va_gc> *&dst_bits = dts->bits; >> vec_safe_reserve_exact (dst_bits, src_bits->length ()); >> for (unsigned i = 0; i < src_bits->length (); ++i) >> dst_bits->quick_push ((*src_bits)[i]); >> @@ -5283,6 +5294,8 @@ read_ipcp_transformation_info (lto_input_block *ib, cgraph_node *node, >> 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_grow_cleared (ts->m_vr, count); >> for (i = 0; i < count; i++) >> { >> @@ -5306,6 +5319,8 @@ read_ipcp_transformation_info (lto_input_block *ib, cgraph_node *node, >> 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_grow_cleared (ts->bits, count); >> >> for (i = 0; i < count; i++) >> diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h >> index 93a2390c..6573a78 100644 >> --- a/gcc/ipa-prop.h >> +++ b/gcc/ipa-prop.h >> @@ -528,6 +528,9 @@ struct GTY(()) ipcp_transformation_summary >> vec<ipa_vr, va_gc> *m_vr; >> }; >> >> +typedef vec<ipa_vr, va_gc> ipa_vr_vec; >> +typedef vec<ipa_bits, va_gc> ipa_bits_vec; >> + >> void ipa_set_node_agg_value_chain (struct cgraph_node *node, >> struct ipa_agg_replacement_value *aggvals); >> void ipcp_grow_transformations_if_necessary (void); >
On 02/02/2017 02:36 AM, kugan wrote:
> This is due to an existing issue. That is, in ipa_node_params_t::remove, m_vr and bits vectors are not set to null such that the gc can claim it.
I've just sent patch that should remove such need as ~ipa_node_params_t should be called
just once:
https://gcc.gnu.org/ml/gcc-patches/2017-02/msg00148.html
Thanks,
Martin
On Thu, Feb 2, 2017 at 1:52 PM, Richard Biener <richard.guenther@gmail.com> wrote: > On Thu, Feb 2, 2017 at 1:48 PM, Jan Hubicka <hubicka@ucw.cz> wrote: >>> >>> 2017-02-02 Kugan Vivekanandarajah <kuganv@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 ()); > > > Why do you need those placement news? Ah, I see we handle finalization but not construction in ggc_[cleared_]alloc... > Richard. > >>> 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) >>> + { >>> + 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. >> >> Patch is OK with that change. >> >> Honza >>> } >>> >>> /* Hook that is called by summary when a node is duplicated. */ >>> @@ -3811,8 +3818,10 @@ ipa_node_params_t::duplicate(cgraph_node *src, cgraph_node *dst, >>> ipcp_grow_transformations_if_necessary (); >>> src_trans = ipcp_get_transformation_summary (src); >>> const vec<ipa_vr, va_gc> *src_vr = src_trans->m_vr; >>> - vec<ipa_vr, va_gc> *&dst_vr >>> - = ipcp_get_transformation_summary (dst)->m_vr; >>> + ipcp_transformation_summary *dts = ipcp_get_transformation_summary (dst); >>> + if (!dts->m_vr) >>> + dts->m_vr = (new (ggc_cleared_alloc<ipa_vr_vec> ()) ipa_vr_vec ()); >>> + vec<ipa_vr, va_gc> *&dst_vr = dts->m_vr; >>> if (vec_safe_length (src_trans->m_vr) > 0) >>> { >>> vec_safe_reserve_exact (dst_vr, src_vr->length ()); >>> @@ -3826,8 +3835,10 @@ ipa_node_params_t::duplicate(cgraph_node *src, cgraph_node *dst, >>> ipcp_grow_transformations_if_necessary (); >>> src_trans = ipcp_get_transformation_summary (src); >>> const vec<ipa_bits, va_gc> *src_bits = src_trans->bits; >>> - vec<ipa_bits, va_gc> *&dst_bits >>> - = ipcp_get_transformation_summary (dst)->bits; >>> + ipcp_transformation_summary *dts = ipcp_get_transformation_summary (dst); >>> + if (!dts->bits) >>> + dts->bits = (new (ggc_cleared_alloc<ipa_bits_vec> ()) ipa_bits_vec ()); >>> + vec<ipa_bits, va_gc> *&dst_bits = dts->bits; >>> vec_safe_reserve_exact (dst_bits, src_bits->length ()); >>> for (unsigned i = 0; i < src_bits->length (); ++i) >>> dst_bits->quick_push ((*src_bits)[i]); >>> @@ -5283,6 +5294,8 @@ read_ipcp_transformation_info (lto_input_block *ib, cgraph_node *node, >>> 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_grow_cleared (ts->m_vr, count); >>> for (i = 0; i < count; i++) >>> { >>> @@ -5306,6 +5319,8 @@ read_ipcp_transformation_info (lto_input_block *ib, cgraph_node *node, >>> 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_grow_cleared (ts->bits, count); >>> >>> for (i = 0; i < count; i++) >>> diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h >>> index 93a2390c..6573a78 100644 >>> --- a/gcc/ipa-prop.h >>> +++ b/gcc/ipa-prop.h >>> @@ -528,6 +528,9 @@ struct GTY(()) ipcp_transformation_summary >>> vec<ipa_vr, va_gc> *m_vr; >>> }; >>> >>> +typedef vec<ipa_vr, va_gc> ipa_vr_vec; >>> +typedef vec<ipa_bits, va_gc> ipa_bits_vec; >>> + >>> void ipa_set_node_agg_value_chain (struct cgraph_node *node, >>> struct ipa_agg_replacement_value *aggvals); >>> void ipcp_grow_transformations_if_necessary (void); >>
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 <kuganv@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
diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h index 93a2390c..acab2aa 100644 --- a/gcc/ipa-prop.h +++ b/gcc/ipa-prop.h @@ -157,13 +157,15 @@ struct GTY(()) ipa_bits }; /* Info about value ranges. */ -struct GTY(()) ipa_vr +struct GTY ((variable_size)) ipa_vr { /* The data fields below are valid only if known is true. */ bool known; enum value_range_type type; - wide_int min; - wide_int max; + /* Minimum and maximum. */ + TRAILING_WIDE_INT_ACCESSOR (min, ints, 0) + TRAILING_WIDE_INT_ACCESSOR (max, ints, 1) + trailing_wide_ints <2> ints; }; /* A jump function for a callsite represents the values passed as actual @@ -525,7 +527,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; }; void ipa_set_node_agg_value_chain (struct cgraph_node *node, However, I am running into error when I do LTO bootstrap that memory seems to have deallocated by the garbage collector. Since we have the reference to the memory allocated by ggc_internal_alloc in the vector (m_vr), I thought it will not be deallocated. But during the bootstrap, when in ipa_node_params_t::duplicate, it seems to have been deallocated as shown in the back trace. I dont understand internals of gc in gcc so any help is appreciated. lto1: internal compiler error: Segmentation fault 0xdedc4b crash_signal ../../gcc/gcc/toplev.c:333 0xb46680 ipa_node_params_t::duplicate(cgraph_node*, cgraph_node*, ipa_node_params*, ipa_node_params*) ../../gcc/gcc/ipa-prop.c:3819 0xb306a3 function_summary<ipa_node_params*>::symtab_duplication(cgraph_node*, cgraph_node*, void*) ../../gcc/gcc/symbol-summary.h:187 0x85aba7 symbol_table::call_cgraph_duplication_hooks(cgraph_node*, cgraph_node*) ../../gcc/gcc/cgraph.c:488 0x8765bf cgraph_node::create_clone(tree_node*, long, int, bool, vec<cgraph_edge*, va_heap, vl_ptr>, bool, cgraph_node*, bitmap_head*, char const*) ../../gcc/gcc/cgraphclones.c:522 0x166fb3b clone_inlined_nodes(cgraph_edge*, bool, bool, int*, int) ../../gcc/gcc/ipa-inline-transform.c:227 0x166fbd7 clone_inlined_nodes(cgraph_edge*, bool, bool, int*, int) ../../gcc/gcc/ipa-inline-transform.c:242 0x1670893 inline_call(cgraph_edge*, bool, vec<cgraph_edge*, va_heap, vl_ptr>*, int*, bool, bool*) ../../gcc/gcc/ipa-inline-transform.c:449 0x1665bd3 inline_small_functions ../../gcc/gcc/ipa-inline.c:2024 0x1667157 ipa_inline ../../gcc/gcc/ipa-inline.c:2434 0x1667fa7 execute ../../gcc/gcc/ipa-inline.c:2845 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
Hi All, As suggested by Richard in the PR, I tried to implement variable size structures for VR as shown in attached patch. That is, I changed ipa-prop.h to: variable structure. Thanks, Kugan diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c index aa3c997..bc6670f 100644 --- a/gcc/ipa-cp.c +++ b/gcc/ipa-cp.c @@ -4945,21 +4945,29 @@ ipcp_store_vr_results (void) for (unsigned i = 0; i < count; i++) { ipcp_param_lattices *plats = ipa_get_parm_lattices (info, i); - ipa_vr vr; + ipa_vr *vr; if (!plats->m_value_range.bottom_p () && !plats->m_value_range.top_p ()) { - vr.known = true; - vr.type = plats->m_value_range.m_vr.type; - vr.min = plats->m_value_range.m_vr.min; - vr.max = plats->m_value_range.m_vr.max; + wide_int min = plats->m_value_range.m_vr.min; + wide_int max = plats->m_value_range.m_vr.max; + unsigned int precision = min.get_precision (); + size_t size = (sizeof (ipa_vr) + + trailing_wide_ints <2>::extra_size (precision)); + vr = static_cast<ipa_vr *> (ggc_internal_alloc (size)); + vr->ints.set_precision (precision); + vr->known = true; + vr->type = plats->m_value_range.m_vr.type; + vr->set_min (min); + vr->set_max (max); } else { - vr.known = false; - vr.type = VR_VARYING; - vr.min = vr.max = wi::zero (INT_TYPE_SIZE); + size_t size = (sizeof (ipa_vr)); + vr = static_cast<ipa_vr *> (ggc_internal_alloc (size)); + vr->known = false; + vr->type = VR_VARYING; } ts->m_vr->quick_push (vr); } diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c index 834c27d..98e8a1b 100644 --- a/gcc/ipa-prop.c +++ b/gcc/ipa-prop.c @@ -3810,14 +3810,37 @@ ipa_node_params_t::duplicate(cgraph_node *src, cgraph_node *dst, { ipcp_grow_transformations_if_necessary (); src_trans = ipcp_get_transformation_summary (src); - const vec<ipa_vr, va_gc> *src_vr = src_trans->m_vr; - vec<ipa_vr, va_gc> *&dst_vr + vec<ipa_vr *, va_gc> *src_vr = src_trans->m_vr; + vec<ipa_vr *, va_gc> *dst_vr = ipcp_get_transformation_summary (dst)->m_vr; if (vec_safe_length (src_trans->m_vr) > 0) { vec_safe_reserve_exact (dst_vr, src_vr->length ()); for (unsigned i = 0; i < src_vr->length (); ++i) - dst_vr->quick_push ((*src_vr)[i]); + { + ipa_vr *dst1, *src1 = (*src_vr)[i]; + if (src1->known) + { + size_t size; + unsigned int precision = src1->get_min ().get_precision (); + size = (sizeof (ipa_vr) + + trailing_wide_ints <2>::extra_size (precision)); + dst1 = static_cast<ipa_vr *> (ggc_internal_alloc (size)); + dst1->ints.set_precision (precision); + dst1->known = true; + dst1->type = src1->type; + dst1->set_min (src1->get_min ()); + dst1->set_max (src1->get_max ()); + } + else + { + size_t size = (sizeof (ipa_vr)); + dst1 = static_cast<ipa_vr *> (ggc_internal_alloc (size)); + dst1->known = false; + dst1->type = src1->type; + } + dst_vr->quick_push (dst1); + } } } @@ -5213,7 +5236,7 @@ write_ipcp_transformation_info (output_block *ob, cgraph_node *node) for (unsigned i = 0; i < count; ++i) { struct bitpack_d bp; - ipa_vr *parm_vr = &(*ts->m_vr)[i]; + ipa_vr *parm_vr = (*ts->m_vr)[i]; bp = bitpack_create (ob->main_stream); bp_pack_value (&bp, parm_vr->known, 1); streamer_write_bitpack (&bp); @@ -5221,8 +5244,8 @@ write_ipcp_transformation_info (output_block *ob, cgraph_node *node) { streamer_write_enum (ob->main_stream, value_rang_type, VR_LAST, parm_vr->type); - streamer_write_wide_int (ob, parm_vr->min); - streamer_write_wide_int (ob, parm_vr->max); + streamer_write_wide_int (ob, parm_vr->get_min ()); + streamer_write_wide_int (ob, parm_vr->get_max ()); } } } @@ -5283,21 +5306,38 @@ read_ipcp_transformation_info (lto_input_block *ib, cgraph_node *node, ipcp_grow_transformations_if_necessary (); ipcp_transformation_summary *ts = ipcp_get_transformation_summary (node); - vec_safe_grow_cleared (ts->m_vr, count); + vec_safe_reserve_exact (ts->m_vr, count); for (i = 0; i < count; i++) { ipa_vr *parm_vr; - parm_vr = &(*ts->m_vr)[i]; struct bitpack_d bp; bp = streamer_read_bitpack (ib); - parm_vr->known = bp_unpack_value (&bp, 1); - if (parm_vr->known) + bool known = bp_unpack_value (&bp, 1); + if (known) + { + enum value_range_type type = streamer_read_enum (ib, + value_range_type, + VR_LAST); + wide_int min = streamer_read_wide_int (ib); + wide_int max = streamer_read_wide_int (ib); + unsigned int precision = min.get_precision (); + size_t size = (sizeof (ipa_vr) + + trailing_wide_ints <2>::extra_size (precision)); + parm_vr = static_cast<ipa_vr *> (ggc_internal_alloc (size)); + (parm_vr)->ints.set_precision (precision); + (parm_vr)->known = known; + (parm_vr)->type = type; + (parm_vr)->set_min (min); + (parm_vr)->set_max (max); + } + else { - parm_vr->type = streamer_read_enum (ib, value_range_type, - VR_LAST); - parm_vr->min = streamer_read_wide_int (ib); - parm_vr->max = streamer_read_wide_int (ib); + size_t size = (sizeof (ipa_vr)); + parm_vr = static_cast<ipa_vr *> (ggc_internal_alloc (size)); + (parm_vr)->known = known; + (parm_vr)->type = VR_VARYING; } + ts->m_vr->quick_push (parm_vr); } } count = streamer_read_uhwi (ib); @@ -5673,7 +5713,7 @@ ipcp_update_vr (struct cgraph_node *node) ipcp_transformation_summary *ts = ipcp_get_transformation_summary (node); if (!ts || vec_safe_length (ts->m_vr) == 0) return; - const vec<ipa_vr, va_gc> &vr = *ts->m_vr; + const vec<ipa_vr *, va_gc> &vr = *ts->m_vr; unsigned count = vr.length (); for (unsigned i = 0; i < count; ++i, parm = next_parm) @@ -5688,8 +5728,8 @@ ipcp_update_vr (struct cgraph_node *node) if (!ddef || !is_gimple_reg (parm)) continue; - if (vr[i].known - && (vr[i].type == VR_RANGE || vr[i].type == VR_ANTI_RANGE)) + if (vr[i]->known + && (vr[i]->type == VR_RANGE || vr[i]->type == VR_ANTI_RANGE)) { tree type = TREE_TYPE (ddef); unsigned prec = TYPE_PRECISION (type); @@ -5699,22 +5739,22 @@ ipcp_update_vr (struct cgraph_node *node) { fprintf (dump_file, "Setting value range of param %u ", i); fprintf (dump_file, "%s[", - (vr[i].type == VR_ANTI_RANGE) ? "~" : ""); - print_decs (vr[i].min, dump_file); + (vr[i]->type == VR_ANTI_RANGE) ? "~" : ""); + print_decs (vr[i]->get_min (), dump_file); fprintf (dump_file, ", "); - print_decs (vr[i].max, dump_file); + print_decs (vr[i]->get_max (), dump_file); fprintf (dump_file, "]\n"); } - set_range_info (ddef, vr[i].type, - wide_int_storage::from (vr[i].min, prec, + set_range_info (ddef, vr[i]->type, + wide_int_storage::from (vr[i]->get_min (), prec, TYPE_SIGN (type)), - wide_int_storage::from (vr[i].max, prec, + wide_int_storage::from (vr[i]->get_max (), prec, TYPE_SIGN (type))); } else if (POINTER_TYPE_P (TREE_TYPE (ddef)) - && vr[i].type == VR_ANTI_RANGE - && wi::eq_p (vr[i].min, 0) - && wi::eq_p (vr[i].max, 0)) + && vr[i]->type == VR_ANTI_RANGE + && wi::eq_p (vr[i]->get_min (), 0) + && wi::eq_p (vr[i]->get_max (), 0)) { if (dump_file) fprintf (dump_file, "Setting nonnull for %u\n", i); diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h index 93a2390c..acab2aa 100644 --- a/gcc/ipa-prop.h +++ b/gcc/ipa-prop.h @@ -157,13 +157,15 @@ struct GTY(()) ipa_bits }; /* Info about value ranges. */ -struct GTY(()) ipa_vr +struct GTY ((variable_size)) ipa_vr { /* The data fields below are valid only if known is true. */ bool known; enum value_range_type type; - wide_int min; - wide_int max; + /* Minimum and maximum. */ + TRAILING_WIDE_INT_ACCESSOR (min, ints, 0) + TRAILING_WIDE_INT_ACCESSOR (max, ints, 1) + trailing_wide_ints <2> ints; }; /* A jump function for a callsite represents the values passed as actual @@ -525,7 +527,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; }; void ipa_set_node_agg_value_chain (struct cgraph_node *node,