[COMMITED,lto] ipcp don't propagate where not needed

Commited with suggested changes.

---------------------------------------------------------------------

This patch disables propagation of ipcp information into partitions
where all instances of the node are marked to be inlined.

Motivation:
Incremental LTO needs stable values between compilations to be
effective. This requirement fails with following example:

void heavily_used_function(int);
...
heavily_used_function(__LINE__);

Ipcp creates long list of all __LINE__ arguments, and then
propagates it with every function clone, even though for inlined
functions this information is not useful.

gcc/ChangeLog:

	* ipa-prop.cc (write_ipcp_transformation_info): Disable
	  uneeded value propagation.
	* lto-cgraph.cc (lto_symtab_encoder_encode): Default values.
	(lto_symtab_encoder_always_inlined_p): New.
	(lto_set_symtab_encoder_not_always_inlined): New.
	(add_node_to): Set always inlined.
	* lto-streamer.h (struct lto_encoder_entry): New field.
	(lto_symtab_encoder_always_inlined_p): New.
---
 gcc/ipa-prop.cc    | 12 +++++++++---
 gcc/lto-cgraph.cc  | 47 +++++++++++++++++++++++++++++-----------------
 gcc/lto-streamer.h | 11 +++++++++++
 3 files changed, 50 insertions(+), 20 deletions(-)

On 06/11/24 13:39 +0100, Michal Jires wrote:
>Commited with suggested changes.
>
>---------------------------------------------------------------------
>
>This patch disables propagation of ipcp information into partitions
>where all instances of the node are marked to be inlined.
>
>Motivation:
>Incremental LTO needs stable values between compilations to be
>effective. This requirement fails with following example:
>
>void heavily_used_function(int);
>...
>heavily_used_function(__LINE__);
>
>Ipcp creates long list of all __LINE__ arguments, and then
>propagates it with every function clone, even though for inlined
>functions this information is not useful.
>
>gcc/ChangeLog:
>
>	* ipa-prop.cc (write_ipcp_transformation_info): Disable
>	  uneeded value propagation.
>	* lto-cgraph.cc (lto_symtab_encoder_encode): Default values.
>	(lto_symtab_encoder_always_inlined_p): New.
>	(lto_set_symtab_encoder_not_always_inlined): New.
>	(add_node_to): Set always inlined.
>	* lto-streamer.h (struct lto_encoder_entry): New field.
>	(lto_symtab_encoder_always_inlined_p): New.
>---
> gcc/ipa-prop.cc    | 12 +++++++++---
> gcc/lto-cgraph.cc  | 47 +++++++++++++++++++++++++++++-----------------
> gcc/lto-streamer.h | 11 +++++++++++
> 3 files changed, 50 insertions(+), 20 deletions(-)
>
>diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc
>index 032358fde22..c5633796721 100644
>--- a/gcc/ipa-prop.cc
>+++ b/gcc/ipa-prop.cc
>@@ -5404,9 +5404,15 @@ write_ipcp_transformation_info (output_block *ob, cgraph_node *node,
>       streamer_write_bitpack (&bp);
>     }
>
>-  streamer_write_uhwi (ob, vec_safe_length (ts->m_vr));
>-  for (const ipa_vr &parm_vr : ts->m_vr)
>-    parm_vr.streamer_write (ob);
>+  /* If all instances of this node are inlined, ipcp info is not useful.  */
>+  if (!lto_symtab_encoder_only_for_inlining_p (encoder, node))
>+    {
>+      streamer_write_uhwi (ob, vec_safe_length (ts->m_vr));
>+      for (const ipa_vr &parm_vr : ts->m_vr)
>+	parm_vr.streamer_write (ob);
>+    }
>+  else
>+    streamer_write_uhwi (ob, 0);
> }
>
> /* Stream in the aggregate value replacement chain for NODE from IB.  */
>diff --git a/gcc/lto-cgraph.cc b/gcc/lto-cgraph.cc
>index 53cc965bdfd..c66f32852a7 100644
>--- a/gcc/lto-cgraph.cc
>+++ b/gcc/lto-cgraph.cc
>@@ -114,7 +114,7 @@ lto_symtab_encoder_encode (lto_symtab_encoder_t encoder,
>
>   if (!encoder->map)
>     {
>-      lto_encoder_entry entry = {node, false, false, false};
>+      lto_encoder_entry entry (node);
>
>       ref = encoder->nodes.length ();
>       encoder->nodes.safe_push (entry);
>@@ -124,7 +124,7 @@ lto_symtab_encoder_encode (lto_symtab_encoder_t encoder,
>   size_t *slot = encoder->map->get (node);
>   if (!slot || !*slot)
>     {
>-      lto_encoder_entry entry = {node, false, false, false};
>+      lto_encoder_entry entry (node);
>       ref = encoder->nodes.length ();
>       if (!slot)
>         encoder->map->put (node, ref + 1);
>@@ -168,6 +168,15 @@ lto_symtab_encoder_delete_node (lto_symtab_encoder_t encoder,
>   return true;
> }
>
>+/* Return TRUE if the NODE and its clones are always inlined.  */
>+
>+bool
>+lto_symtab_encoder_only_for_inlining_p (lto_symtab_encoder_t encoder,
>+					struct cgraph_node *node)
>+{
>+  int index = lto_symtab_encoder_lookup (encoder, node);
>+  return encoder->nodes[index].only_for_inlining;
>+}
>
> /* Return TRUE if we should encode the body of NODE (if any).  */
>
>@@ -179,17 +188,6 @@ lto_symtab_encoder_encode_body_p (lto_symtab_encoder_t encoder,
>   return encoder->nodes[index].body;
> }
>
>-/* Specify that we encode the body of NODE in this partition.  */
>-
>-static void
>-lto_set_symtab_encoder_encode_body (lto_symtab_encoder_t encoder,
>-				    struct cgraph_node *node)
>-{
>-  int index = lto_symtab_encoder_encode (encoder, node);
>-  gcc_checking_assert (encoder->nodes[index].node == node);
>-  encoder->nodes[index].body = true;
>-}
>-
> /* Return TRUE if we should encode initializer of NODE (if any).  */
>
> bool
>@@ -799,13 +797,28 @@ output_refs (lto_symtab_encoder_t encoder)
>
> static void
> add_node_to (lto_symtab_encoder_t encoder, struct cgraph_node *node,
>-	     bool include_body)
>+	     bool include_body, bool not_inlined)
> {
>   if (node->clone_of)
>-    add_node_to (encoder, node->clone_of, include_body);
>+    add_node_to (encoder, node->clone_of, include_body, not_inlined);
>+
>+  int index = lto_symtab_encoder_encode (encoder, node);
>+  gcc_checking_assert (encoder->nodes[index].node == node);
>+
>   if (include_body)
>-    lto_set_symtab_encoder_encode_body (encoder, node);
>-  lto_symtab_encoder_encode (encoder, node);
>+    encoder->nodes[index].body = true;
>+  if (not_inlined)
>+    encoder->nodes[index].only_for_inlining = false;
>+}
>+
>+/* Add NODE into encoder as well as nodes it is cloned from.
>+   Do it in a way so clones appear first.  */
>+
>+static void
>+add_node_to (lto_symtab_encoder_t encoder, struct cgraph_node *node,
>+	     bool include_body)
>+{
>+  add_node_to (encoder, node, include_body, include_body && !node->inlined_to);
> }
>
> /* Add all references in NODE to encoders.  */
>diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h
>index 397f5fc8d68..2f8b842f081 100644
>--- a/gcc/lto-streamer.h
>+++ b/gcc/lto-streamer.h
>@@ -443,12 +443,21 @@ struct lto_stats_d
> /* Entry of LTO symtab encoder.  */
> struct lto_encoder_entry
> {
>+  /* Constructors.  */
>+  lto_encoder_entry () {}

If there's going to be a constructor then it should initialize the members.

Otherwise, your original patch was better, because you could write
this to get an all-zeros object:

   lto_encoder_entry e{};

Now you can't safely initialize it, because the default constructor
leaves everything indeterminate. That's just a bug waiting to happen.

If you want to avoid repetition, just initialize everything in the
default constructor then delegate to that for the other one:

   lto_encoder_entry ()
     : node (nullptr), in_partition (false), body (false), only_for_inlining (true),
       initializer (false)
   {}

   lto_encoder_entry (symtab_node* n)
     : lto_encoder_entry()
   {
     node = n;
   }




>+  lto_encoder_entry (symtab_node* n)
>+    : node (n), in_partition (false), body (false), only_for_inlining (true),
>+      initializer (false)
>+  {}
>+
>   symtab_node *node;
>   /* Is the node in this partition (i.e. ltrans of this partition will
>      be responsible for outputting it)? */
>   unsigned int in_partition:1;
>   /* Do we encode body in this partition?  */
>   unsigned int body:1;
>+  /* Do we stream this node only for inlining?  */
>+  unsigned int only_for_inlining:1;
>   /* Do we encode initializer in this partition?
>      For example the readonly variable initializers are encoded to aid
>      constant folding even if they are not in the partition.  */
>@@ -914,6 +923,8 @@ void lto_symtab_encoder_delete (lto_symtab_encoder_t);
> bool lto_symtab_encoder_delete_node (lto_symtab_encoder_t, symtab_node *);
> bool lto_symtab_encoder_encode_body_p (lto_symtab_encoder_t,
> 				       struct cgraph_node *);
>+bool lto_symtab_encoder_only_for_inlining_p (lto_symtab_encoder_t,
>+					     struct cgraph_node *);
> bool lto_symtab_encoder_in_partition_p (lto_symtab_encoder_t,
> 					symtab_node *);
> void lto_set_symtab_encoder_in_partition (lto_symtab_encoder_t,
>-- 
>2.47.0
>

On Wed, 2024-11-06 at 17:33:50 +0000, Jonathan Wakely wrote:
> 
> If there's going to be a constructor then it should initialize the members.
> 
> Otherwise, your original patch was better, because you could write
> this to get an all-zeros object:
> 
>   lto_encoder_entry e{};
> 
> Now you can't safely initialize it, because the default constructor
> leaves everything indeterminate. That's just a bug waiting to happen.
> 

Using all-zeros would be probably bug anyway and explicitly initializing
might encourage thinking that such default values are supposed to be
used.

Anyway, I have misglanced the code for which this was needed, and we can
trivially get rid of it.

Is this now OK?

---
 gcc/lto-cgraph.cc  | 3 +--
 gcc/lto-streamer.h | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/gcc/lto-cgraph.cc b/gcc/lto-cgraph.cc
index b18d2b34e46..c9b846a04d6 100644
--- a/gcc/lto-cgraph.cc
+++ b/gcc/lto-cgraph.cc
@@ -142,7 +142,6 @@ lto_symtab_encoder_delete_node (lto_symtab_encoder_t encoder,
 			        symtab_node *node)
 {
   int index;
-  lto_encoder_entry last_node;
 
   size_t *slot = encoder->map->get (node);
   if (slot == NULL || !*slot)
@@ -153,7 +152,7 @@ lto_symtab_encoder_delete_node (lto_symtab_encoder_t encoder,
 
   /* Remove from vector. We do this by swapping node with the last element
      of the vector.  */
-  last_node = encoder->nodes.pop ();
+  lto_encoder_entry last_node = encoder->nodes.pop ();
   if (last_node.node != node)
     {
       bool existed = encoder->map->put (last_node.node, index + 1);
diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h
index 1c416a7a1b9..294e7b3e328 100644
--- a/gcc/lto-streamer.h
+++ b/gcc/lto-streamer.h
@@ -443,8 +443,7 @@ struct lto_stats_d
 /* Entry of LTO symtab encoder.  */
 struct lto_encoder_entry
 {
-  /* Constructors.  */
-  lto_encoder_entry () {}
+  /* Constructor.  */
   lto_encoder_entry (symtab_node* n)
     : node (n), in_partition (false), body (false), only_for_inlining (true),
       initializer (false)

On Wed, 6 Nov 2024 at 18:39, Michal Jires <mjires@suse.cz> wrote:
>
> On Wed, 2024-11-06 at 17:33:50 +0000, Jonathan Wakely wrote:
> >
> > If there's going to be a constructor then it should initialize the members.
> >
> > Otherwise, your original patch was better, because you could write
> > this to get an all-zeros object:
> >
> >   lto_encoder_entry e{};
> >
> > Now you can't safely initialize it, because the default constructor
> > leaves everything indeterminate. That's just a bug waiting to happen.
> >
>
> Using all-zeros would be probably bug anyway and explicitly initializing
> might encourage thinking that such default values are supposed to be
> used.
>
> Anyway, I have misglanced the code for which this was needed, and we can
> trivially get rid of it.
>
> Is this now OK?

I can't approve it, but I like this a lot more.

Initialize at the point of use, nto at the top of the function when we
don't have the values to initialize it yet.

>
> ---
>  gcc/lto-cgraph.cc  | 3 +--
>  gcc/lto-streamer.h | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/lto-cgraph.cc b/gcc/lto-cgraph.cc
> index b18d2b34e46..c9b846a04d6 100644
> --- a/gcc/lto-cgraph.cc
> +++ b/gcc/lto-cgraph.cc
> @@ -142,7 +142,6 @@ lto_symtab_encoder_delete_node (lto_symtab_encoder_t encoder,
>                                 symtab_node *node)
>  {
>    int index;
> -  lto_encoder_entry last_node;
>
>    size_t *slot = encoder->map->get (node);
>    if (slot == NULL || !*slot)
> @@ -153,7 +152,7 @@ lto_symtab_encoder_delete_node (lto_symtab_encoder_t encoder,
>
>    /* Remove from vector. We do this by swapping node with the last element
>       of the vector.  */
> -  last_node = encoder->nodes.pop ();
> +  lto_encoder_entry last_node = encoder->nodes.pop ();
>    if (last_node.node != node)
>      {
>        bool existed = encoder->map->put (last_node.node, index + 1);
> diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h
> index 1c416a7a1b9..294e7b3e328 100644
> --- a/gcc/lto-streamer.h
> +++ b/gcc/lto-streamer.h
> @@ -443,8 +443,7 @@ struct lto_stats_d
>  /* Entry of LTO symtab encoder.  */
>  struct lto_encoder_entry
>  {
> -  /* Constructors.  */
> -  lto_encoder_entry () {}
> +  /* Constructor.  */
>    lto_encoder_entry (symtab_node* n)
>      : node (n), in_partition (false), body (false), only_for_inlining (true),
>        initializer (false)
> --
> 2.47.0
>