[RFC] Bug lto/78140

Message ID c8477826-61d1-7b44-d3e3-364fa1a52588@linaro.org
State New
Headers show

Commit Message

kugan Jan. 29, 2017, 11:23 p.m.
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,

Comments

Richard Biener Jan. 30, 2017, 10:08 a.m. | #1
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
kugan Feb. 2, 2017, 1:36 a.m. | #2
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);

kugan Feb. 2, 2017, 2:59 a.m. | #3
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


Jan Hubicka Feb. 2, 2017, 12:48 p.m. | #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);
Richard Biener Feb. 2, 2017, 12:52 p.m. | #5
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);

>
Martin Liška Feb. 2, 2017, 12:55 p.m. | #6
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
Richard Biener Feb. 2, 2017, 12:57 p.m. | #7
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);

>>
Martin Jambor Feb. 2, 2017, 4:02 p.m. | #8
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

Patch hide | download patch | download mbox

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