[03/12] ipa-cp: Write transformation summaries of all functions

  Hi,

IPA-CP transformation summary streaming code currently won't stream
out transformations necessary for clones which are only necessary for
materialization of other clones (such as an IPA-CP clone which is then
cloned again by IPA-SRA).  However, a follow-up patch for bettor
reconciling IPA-SRA and IPA-CP modifications requires to have that
information at its disposal and so this one reworks the streaming to
write out all non-empty transformation summaries.

This should actually mean less streaming in typical case because
previously we streamed three zeros for all nodes in a partition with
no useful information associated with them.  Currently we don't stream
anything for those.

When reworking the streaming, I also simplified it a little a
converted it writing to nicer C++ vector iterations.

Bootstrapped and tested on x86_64-linux.  OK for master?

Thanks,

Martin


gcc/ChangeLog:

2022-11-11  Martin Jambor  <mjambor@suse.cz>

	* ipa-prop.cc (useful_ipcp_transformation_info_p): New function.
	(write_ipcp_transformation_info): Added a parameter, simplified
	given that is known not to be NULL.
	(ipcp_write_transformation_summaries): Write out all useful
	transformation summaries.
	(read_ipcp_transformation_info): Simplify given that some info
	will be read.
	(read_replacements_section): Remove assert.
---
 gcc/ipa-prop.cc | 151 +++++++++++++++++++++++-------------------------
 1 file changed, 71 insertions(+), 80 deletions(-)
  

> Hi,
> 
> IPA-CP transformation summary streaming code currently won't stream
> out transformations necessary for clones which are only necessary for
> materialization of other clones (such as an IPA-CP clone which is then
> cloned again by IPA-SRA).  However, a follow-up patch for bettor
> reconciling IPA-SRA and IPA-CP modifications requires to have that
> information at its disposal and so this one reworks the streaming to
> write out all non-empty transformation summaries.
> 
> This should actually mean less streaming in typical case because
> previously we streamed three zeros for all nodes in a partition with
> no useful information associated with them.  Currently we don't stream
> anything for those.
> 
> When reworking the streaming, I also simplified it a little a
> converted it writing to nicer C++ vector iterations.
> 
> Bootstrapped and tested on x86_64-linux.  OK for master?
> 
> Thanks,
> 
> Martin
> 
> 
> gcc/ChangeLog:
> 
> 2022-11-11  Martin Jambor  <mjambor@suse.cz>
> 
> 	* ipa-prop.cc (useful_ipcp_transformation_info_p): New function.
> 	(write_ipcp_transformation_info): Added a parameter, simplified
> 	given that is known not to be NULL.
> 	(ipcp_write_transformation_summaries): Write out all useful
> 	transformation summaries.
> 	(read_ipcp_transformation_info): Simplify given that some info
> 	will be read.
> 	(read_replacements_section): Remove assert.
> ---
>  gcc/ipa-prop.cc | 151 +++++++++++++++++++++++-------------------------
>  1 file changed, 71 insertions(+), 80 deletions(-)
> 
> diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc
> index e6cf25591b3..cfd12a97b36 100644
> --- a/gcc/ipa-prop.cc
> +++ b/gcc/ipa-prop.cc
> @@ -5279,80 +5279,72 @@ ipa_prop_read_jump_functions (void)
>      }
>  }
>  
> -void
> -write_ipcp_transformation_info (output_block *ob, cgraph_node *node)
> +/* Return true if the IPA-CP transformation summary TS is non-NULL and contains
> +   useful info.  */
> +static bool
> +useful_ipcp_transformation_info_p (ipcp_transformation *ts)
>  {
> -  int node_ref;
> -  unsigned int count = 0;
> -  lto_symtab_encoder_t encoder;
> +  if (!ts)
> +    return false;
> +  if (!vec_safe_is_empty (ts->m_agg_values)
> +      || !vec_safe_is_empty (ts->bits)
> +      || !vec_safe_is_empty (ts->m_vr))
> +    return true;
> +  return false;
> +}

This way we stream transformation info for everything in the boundary.
Even for functions that belongs to other partitions and they are called
from current partition.  Perhaps we want to check that the function has
body streamed?

Also would be possible to make a testcase? I think it should be possible
to use -flto-partition=max and builtin_constant_p checks verifying that
intended transformation happens.

Honza
  

Hi,

On Wed, Nov 16 2022, Jan Hubicka wrote:
>> IPA-CP transformation summary streaming code currently won't stream
>> out transformations necessary for clones which are only necessary for
>> materialization of other clones (such as an IPA-CP clone which is then
>> cloned again by IPA-SRA).  However, a follow-up patch for bettor
>> reconciling IPA-SRA and IPA-CP modifications requires to have that
>> information at its disposal and so this one reworks the streaming to
>> write out all non-empty transformation summaries.
>> 
>> This should actually mean less streaming in typical case because
>> previously we streamed three zeros for all nodes in a partition with
>> no useful information associated with them.  Currently we don't stream
>> anything for those.
>> 
>> When reworking the streaming, I also simplified it a little a
>> converted it writing to nicer C++ vector iterations.
>> 
>> Bootstrapped and tested on x86_64-linux.  OK for master?
>> 
>> Thanks,
>> 
>> Martin
>> 
>> 
>> gcc/ChangeLog:
>> 
>> 2022-11-11  Martin Jambor  <mjambor@suse.cz>
>> 
>> 	* ipa-prop.cc (useful_ipcp_transformation_info_p): New function.
>> 	(write_ipcp_transformation_info): Added a parameter, simplified
>> 	given that is known not to be NULL.
>> 	(ipcp_write_transformation_summaries): Write out all useful
>> 	transformation summaries.
>> 	(read_ipcp_transformation_info): Simplify given that some info
>> 	will be read.
>> 	(read_replacements_section): Remove assert.
>> ---
>>  gcc/ipa-prop.cc | 151 +++++++++++++++++++++++-------------------------
>>  1 file changed, 71 insertions(+), 80 deletions(-)
>> 
>> diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc
>> index e6cf25591b3..cfd12a97b36 100644
>> --- a/gcc/ipa-prop.cc
>> +++ b/gcc/ipa-prop.cc
>> @@ -5279,80 +5279,72 @@ ipa_prop_read_jump_functions (void)
>>      }
>>  }
>>  
>> -void
>> -write_ipcp_transformation_info (output_block *ob, cgraph_node *node)
>> +/* Return true if the IPA-CP transformation summary TS is non-NULL and contains
>> +   useful info.  */
>> +static bool
>> +useful_ipcp_transformation_info_p (ipcp_transformation *ts)
>>  {
>> -  int node_ref;
>> -  unsigned int count = 0;
>> -  lto_symtab_encoder_t encoder;
>> +  if (!ts)
>> +    return false;
>> +  if (!vec_safe_is_empty (ts->m_agg_values)
>> +      || !vec_safe_is_empty (ts->bits)
>> +      || !vec_safe_is_empty (ts->m_vr))
>> +    return true;
>> +  return false;
>> +}
>
> This way we stream transformation info for everything in the boundary.
> Even for functions that belongs to other partitions and they are called
> from current partition.  Perhaps we want to check that the function has
> body streamed?
>

Please see the updated patch below, which has survived bootstrap,
LTO-bootstrap and testing.

> Also would be possible to make a testcase? I think it should be possible
> to use -flto-partition=max and builtin_constant_p checks verifying that
> intended transformation happens.
>

make -k check-gcc RUNTESTFLAGS="execute.exp=20001017-2.c"

is a testcase which ICEs if IPA-CP info is not streamed and the
subsequent patch is applied.

Thanks,

Martin




IPA-CP transformation summary streaming code currently won't stream
out transformations necessary for clones which are only necessary for
materialization of other clones (such as an IPA-CP clone which is then
cloned again by IPA-SRA).  However, a follow-up patch for bettor
reconciling IPA-SRA and IPA-CP modifications requires to have that
information at its disposal and so this one reworks the streaming to
write out all non-empty transformation summaries.

In order not to stream transformation summaies into partitions where
the node itself nor any of its clones are materialized, I had to make
sure that clones also get encode_body flag in the encoder (so that it
could be tested) and therefore in turn lto_output understands it needs
to skip clones.

This should actually mean less streaming in typical case because
previously we streamed three zeros for all nodes in a partition with
no useful information associated with them.  Currently we don't stream
anything for those.

When reworking the streaming, I also simplified it a little a
converted it writing to nicer C++ vector iterations.

gcc/ChangeLog:

2022-11-25  Martin Jambor  <mjambor@suse.cz>

	* ipa-prop.cc (useful_ipcp_transformation_info_p): New function.
	(write_ipcp_transformation_info): Added a parameter, simplified
	given that is known not to be NULL.
	(ipcp_write_transformation_summaries): Write out all useful
	transformation summaries.
	(read_ipcp_transformation_info): Simplify given that some info
	will be read.
	(read_replacements_section): Remove assert.
	* lto-cgraph.cc (add_node_to): Also set encode_body for clones.
	* lto-streamer-out.cc (lto_output): Do not output virtual clones.
---
 gcc/ipa-prop.cc         | 153 +++++++++++++++++++---------------------
 gcc/lto-cgraph.cc       |   2 +-
 gcc/lto-streamer-out.cc |   3 +-
 3 files changed, 76 insertions(+), 82 deletions(-)

diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc
index e6cf25591b3..fcadf64ead7 100644
--- a/gcc/ipa-prop.cc
+++ b/gcc/ipa-prop.cc
@@ -5279,80 +5279,72 @@ ipa_prop_read_jump_functions (void)
     }
 }
 
-void
-write_ipcp_transformation_info (output_block *ob, cgraph_node *node)
+/* Return true if the IPA-CP transformation summary TS is non-NULL and contains
+   useful info.  */
+static bool
+useful_ipcp_transformation_info_p (ipcp_transformation *ts)
 {
-  int node_ref;
-  unsigned int count = 0;
-  lto_symtab_encoder_t encoder;
+  if (!ts)
+    return false;
+  if (!vec_safe_is_empty (ts->m_agg_values)
+      || !vec_safe_is_empty (ts->bits)
+      || !vec_safe_is_empty (ts->m_vr))
+    return true;
+  return false;
+}
 
-  encoder = ob->decl_state->symtab_node_encoder;
-  node_ref = lto_symtab_encoder_encode (encoder, node);
+/* Write into OB IPA-CP transfromation summary TS describing NODE.  */
+
+void
+write_ipcp_transformation_info (output_block *ob, cgraph_node *node,
+				ipcp_transformation *ts)
+{
+  lto_symtab_encoder_t encoder = ob->decl_state->symtab_node_encoder;
+  int node_ref = lto_symtab_encoder_encode (encoder, node);
   streamer_write_uhwi (ob, node_ref);
 
-  ipcp_transformation *ts = ipcp_get_transformation_summary (node);
-  if (ts && !vec_safe_is_empty (ts->m_agg_values))
+  streamer_write_uhwi (ob, vec_safe_length (ts->m_agg_values));
+  for (const ipa_argagg_value &av : ts->m_agg_values)
     {
-      streamer_write_uhwi (ob, ts->m_agg_values->length ());
-      for (const ipa_argagg_value &av : ts->m_agg_values)
+      struct bitpack_d bp;
+
+      stream_write_tree (ob, av.value, true);
+      streamer_write_uhwi (ob, av.unit_offset);
+      streamer_write_uhwi (ob, av.index);
+
+      bp = bitpack_create (ob->main_stream);
+      bp_pack_value (&bp, av.by_ref, 1);
+      streamer_write_bitpack (&bp);
+    }
+
+  streamer_write_uhwi (ob, vec_safe_length (ts->m_vr));
+  for (const ipa_vr &parm_vr : ts->m_vr)
+    {
+      struct bitpack_d bp;
+      bp = bitpack_create (ob->main_stream);
+      bp_pack_value (&bp, parm_vr.known, 1);
+      streamer_write_bitpack (&bp);
+      if (parm_vr.known)
 	{
-	  struct bitpack_d bp;
-
-	  stream_write_tree (ob, av.value, true);
-	  streamer_write_uhwi (ob, av.unit_offset);
-	  streamer_write_uhwi (ob, av.index);
-
-	  bp = bitpack_create (ob->main_stream);
-	  bp_pack_value (&bp, av.by_ref, 1);
-	  streamer_write_bitpack (&bp);
+	  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);
 	}
     }
-  else
-    streamer_write_uhwi (ob, 0);
 
-  if (ts && vec_safe_length (ts->m_vr) > 0)
+  streamer_write_uhwi (ob, vec_safe_length (ts->bits));
+  for (const ipa_bits *bits_jfunc : ts->bits)
     {
-      count = ts->m_vr->length ();
-      streamer_write_uhwi (ob, count);
-      for (unsigned i = 0; i < count; ++i)
+      struct bitpack_d bp = bitpack_create (ob->main_stream);
+      bp_pack_value (&bp, !!bits_jfunc, 1);
+      streamer_write_bitpack (&bp);
+      if (bits_jfunc)
 	{
-	  struct bitpack_d bp;
-	  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,
-				   VR_LAST, parm_vr->type);
-	      streamer_write_wide_int (ob, parm_vr->min);
-	      streamer_write_wide_int (ob, parm_vr->max);
-	    }
+	  streamer_write_widest_int (ob, bits_jfunc->value);
+	  streamer_write_widest_int (ob, bits_jfunc->mask);
 	}
     }
-  else
-    streamer_write_uhwi (ob, 0);
-
-  if (ts && vec_safe_length (ts->bits) > 0)
-    {
-      count = ts->bits->length ();
-      streamer_write_uhwi (ob, count);
-
-      for (unsigned i = 0; i < count; ++i)
-	{
-	  const ipa_bits *bits_jfunc = (*ts->bits)[i];
-	  struct bitpack_d bp = bitpack_create (ob->main_stream);
-	  bp_pack_value (&bp, !!bits_jfunc, 1);
-	  streamer_write_bitpack (&bp);
-	  if (bits_jfunc)
-	    {
-	      streamer_write_widest_int (ob, bits_jfunc->value);
-	      streamer_write_widest_int (ob, bits_jfunc->mask);
-	    }
-	}
-    }
-  else
-    streamer_write_uhwi (ob, 0);
 }
 
 /* Stream in the aggregate value replacement chain for NODE from IB.  */
@@ -5362,12 +5354,12 @@ read_ipcp_transformation_info (lto_input_block *ib, cgraph_node *node,
 			       data_in *data_in)
 {
   unsigned int count, i;
+  ipcp_transformation_initialize ();
+  ipcp_transformation *ts = ipcp_transformation_sum->get_create (node);
 
   count = streamer_read_uhwi (ib);
   if (count > 0)
     {
-      ipcp_transformation_initialize ();
-      ipcp_transformation *ts = ipcp_transformation_sum->get_create (node);
       vec_safe_grow_cleared (ts->m_agg_values, count, true);
       for (i = 0; i <count; i++)
 	{
@@ -5385,8 +5377,6 @@ read_ipcp_transformation_info (lto_input_block *ib, cgraph_node *node,
   count = streamer_read_uhwi (ib);
   if (count > 0)
     {
-      ipcp_transformation_initialize ();
-      ipcp_transformation *ts = ipcp_transformation_sum->get_create (node);
       vec_safe_grow_cleared (ts->m_vr, count, true);
       for (i = 0; i < count; i++)
 	{
@@ -5407,10 +5397,7 @@ read_ipcp_transformation_info (lto_input_block *ib, cgraph_node *node,
   count = streamer_read_uhwi (ib);
   if (count > 0)
     {
-      ipcp_transformation_initialize ();
-      ipcp_transformation *ts = ipcp_transformation_sum->get_create (node);
       vec_safe_grow_cleared (ts->bits, count, true);
-
       for (i = 0; i < count; i++)
 	{
 	  struct bitpack_d bp = streamer_read_bitpack (ib);
@@ -5432,31 +5419,38 @@ read_ipcp_transformation_info (lto_input_block *ib, cgraph_node *node,
 void
 ipcp_write_transformation_summaries (void)
 {
-  struct cgraph_node *node;
   struct output_block *ob;
   unsigned int count = 0;
-  lto_symtab_encoder_iterator lsei;
   lto_symtab_encoder_t encoder;
 
   ob = create_output_block (LTO_section_ipcp_transform);
   encoder = ob->decl_state->symtab_node_encoder;
   ob->symbol = NULL;
-  for (lsei = lsei_start_function_in_partition (encoder); !lsei_end_p (lsei);
-       lsei_next_function_in_partition (&lsei))
+
+  for (int i = 0; i < lto_symtab_encoder_size (encoder); i++)
     {
-      node = lsei_cgraph_node (lsei);
-      if (node->has_gimple_body_p ())
+      symtab_node *snode = lto_symtab_encoder_deref (encoder, i);
+      cgraph_node *cnode = dyn_cast <cgraph_node *> (snode);
+      if (!cnode)
+	continue;
+      ipcp_transformation *ts = ipcp_get_transformation_summary (cnode);
+      if (useful_ipcp_transformation_info_p (ts)
+	  && lto_symtab_encoder_encode_body_p (encoder, cnode))
 	count++;
     }
 
   streamer_write_uhwi (ob, count);
 
-  for (lsei = lsei_start_function_in_partition (encoder); !lsei_end_p (lsei);
-       lsei_next_function_in_partition (&lsei))
+  for (int i = 0; i < lto_symtab_encoder_size (encoder); i++)
     {
-      node = lsei_cgraph_node (lsei);
-      if (node->has_gimple_body_p ())
-	write_ipcp_transformation_info (ob, node);
+      symtab_node *snode = lto_symtab_encoder_deref (encoder, i);
+      cgraph_node *cnode = dyn_cast <cgraph_node *> (snode);
+      if (!cnode)
+	continue;
+      ipcp_transformation *ts = ipcp_get_transformation_summary (cnode);
+      if (useful_ipcp_transformation_info_p (ts)
+	  && lto_symtab_encoder_encode_body_p (encoder, cnode))
+	write_ipcp_transformation_info (ob, cnode, ts);
     }
   streamer_write_char_stream (ob->main_stream, 0);
   produce_asm (ob, NULL);
@@ -5497,7 +5491,6 @@ read_replacements_section (struct lto_file_decl_data *file_data,
       encoder = file_data->symtab_node_encoder;
       node = dyn_cast<cgraph_node *> (lto_symtab_encoder_deref (encoder,
 								index));
-      gcc_assert (node->definition);
       read_ipcp_transformation_info (&ib_main, node, data_in);
     }
   lto_free_section_data (file_data, LTO_section_jump_functions, NULL, data,
diff --git a/gcc/lto-cgraph.cc b/gcc/lto-cgraph.cc
index 350195d86db..11079b0f0f0 100644
--- a/gcc/lto-cgraph.cc
+++ b/gcc/lto-cgraph.cc
@@ -797,7 +797,7 @@ add_node_to (lto_symtab_encoder_t encoder, struct cgraph_node *node,
 {
   if (node->clone_of)
     add_node_to (encoder, node->clone_of, include_body);
-  else if (include_body)
+  if (include_body)
     lto_set_symtab_encoder_encode_body (encoder, node);
   lto_symtab_encoder_encode (encoder, node);
 }
diff --git a/gcc/lto-streamer-out.cc b/gcc/lto-streamer-out.cc
index 1e389049062..08f75b09204 100644
--- a/gcc/lto-streamer-out.cc
+++ b/gcc/lto-streamer-out.cc
@@ -2752,7 +2752,8 @@ lto_output (void)
 	continue;
       if (cgraph_node *node = dyn_cast <cgraph_node *> (snode))
 	{
-	  if (lto_symtab_encoder_encode_body_p (encoder, node))
+	  if (lto_symtab_encoder_encode_body_p (encoder, node)
+	      && !node->clone_of)
 	    symbols_to_copy.safe_push (node);
 	}
       else if (varpool_node *node = dyn_cast <varpool_node *> (snode))