[PATCH] Fix PR ipa/113996

[PATCH] Fix PR ipa/113996

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]

Hi,

this is a regression present on all active branches: the attached Ada testcase 
triggers an assertion failure when compiled with -O2 -gnatp -flto:

  /* Initialize the static chain.  */
  p = DECL_STRUCT_FUNCTION (fn)->static_chain_decl;
  gcc_assert (fn != current_function_decl);
  if (p)
    {
      /* No static chain?  Seems like a bug in tree-nested.cc.  */
      gcc_assert (static_chain);                                  <--- here

      setup_one_parameter (id, p, static_chain, fn, bb, &vars);
    }

The problem is that the ICF pass identifies two functions, one of which has a 
static chain but the other does not.  The proposed fix is just to prevent this 
identification from occurring.

Tested on x86-64/Linux, OK for all active branches?


2024-03-11  Eric Botcazou  <ebotcazou@adacore.com>

	PR ipa/113996
	* ipa-icf.h (sem_function): Add static_chain_present member.
	* ipa-icf.cc (sem_function::get_hash): Hash it.
	(sem_function::equals_wpa): Compare it.
	(sem_function::equals_private): Likewise.
	(sem_function::init): Initialize it.


2024-03-11  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/lto27.adb: New test.

-- 
Eric Botcazou

[-- Attachment #2: lto27.adb --]
[-- Type: text/x-adasrc, Size: 409 bytes --]

-- { dg-do link }
-- { dg-options "-O2 -gnatp -flto" { target lto } }

with Ada.Containers.Hashed_Maps;
with Ada.Strings.Hash;

procedure Lto27 is
   subtype Node_Name is String (1 .. 4);

   package Node_Maps is new Ada.Containers.Hashed_Maps
     (Key_Type        => Node_Name,
      Element_Type    => Integer,
      Hash            => Ada.Strings.Hash,
      Equivalent_Keys => "=");

begin
   null;
end;

[-- Attachment #3: pr113996.diff --]
[-- Type: text/x-patch, Size: 2762 bytes --]

diff --git a/gcc/ipa-icf.cc b/gcc/ipa-icf.cc
index 5d5a42f9c6c..dff7ad6efda 100644
--- a/gcc/ipa-icf.cc
+++ b/gcc/ipa-icf.cc
@@ -284,6 +284,7 @@ sem_function::get_hash (void)
       hstate.add_int (177454); /* Random number for function type.  */
 
       hstate.add_int (arg_count);
+      hstate.add_int (static_chain_present);
       hstate.add_int (cfg_checksum);
       hstate.add_int (gcode_hash);
 
@@ -655,7 +656,10 @@ sem_function::equals_wpa (sem_item *item,
     }
 
   if (list1 || list2)
-    return return_false_with_msg ("Mismatched number of parameters");
+    return return_false_with_msg ("mismatched number of parameters");
+
+  if (static_chain_present != m_compared_func->static_chain_present)
+    return return_false_with_msg ("static chain mismatch");
 
   if (node->num_references () != item->node->num_references ())
     return return_false_with_msg ("different number of references");
@@ -876,7 +880,10 @@ sem_function::equals_private (sem_item *item)
         return return_false ();
     }
   if (arg1 || arg2)
-    return return_false_with_msg ("Mismatched number of arguments");
+    return return_false_with_msg ("mismatched number of arguments");
+
+  if (static_chain_present != m_compared_func->static_chain_present)
+    return return_false_with_msg ("static chain mismatch");
 
   if (!dyn_cast <cgraph_node *> (node)->has_gimple_body_p ())
     return true;
@@ -1368,6 +1375,8 @@ sem_function::init (ipa_icf_gimple::func_checker *checker)
   /* iterating all function arguments.  */
   arg_count = count_formal_params (fndecl);
 
+  static_chain_present = func->static_chain_decl != NULL_TREE;
+
   edge_count = n_edges_for_fn (func);
   cgraph_node *cnode = dyn_cast <cgraph_node *> (node);
   if (!cnode->thunk)
diff --git a/gcc/ipa-icf.h b/gcc/ipa-icf.h
index ef7e41bfa88..bd9fd9fb294 100644
--- a/gcc/ipa-icf.h
+++ b/gcc/ipa-icf.h
@@ -355,6 +355,9 @@ public:
      parameters.  */
   bool compatible_parm_types_p (tree, tree);
 
+  /* Return true if parameter I may be used.  */
+  bool param_used_p (unsigned int i);
+
   /* Exception handling region tree.  */
   eh_region region_tree;
 
@@ -379,6 +382,9 @@ public:
   /* Total number of SSA names used in the function.  */
   unsigned ssa_names_size;
 
+  /* Whether the special PARM_DECL for the static chain is present.  */
+  bool static_chain_present;
+
   /* Array of structures for all basic blocks.  */
   vec <ipa_icf_gimple::sem_bb *> bb_sorted;
 
@@ -386,9 +392,6 @@ public:
      function.  */
   hashval_t m_alias_sets_hash;
 
-  /* Return true if parameter I may be used.  */
-  bool param_used_p (unsigned int i);
-
 private:
   /* Calculates hash value based on a BASIC_BLOCK.  */
   hashval_t get_bb_hash (const ipa_icf_gimple::sem_bb *basic_block);

Re: [PATCH] Fix PR ipa/113996

[Jeff Law]

On 3/11/24 4:38 PM, Eric Botcazou wrote:
> Hi,
> 
> this is a regression present on all active branches: the attached Ada testcase
> triggers an assertion failure when compiled with -O2 -gnatp -flto:
> 
>    /* Initialize the static chain.  */
>    p = DECL_STRUCT_FUNCTION (fn)->static_chain_decl;
>    gcc_assert (fn != current_function_decl);
>    if (p)
>      {
>        /* No static chain?  Seems like a bug in tree-nested.cc.  */
>        gcc_assert (static_chain);                                  <--- here
> 
>        setup_one_parameter (id, p, static_chain, fn, bb, &vars);
>      }
> 
> The problem is that the ICF pass identifies two functions, one of which has a
> static chain but the other does not.  The proposed fix is just to prevent this
> identification from occurring.
> 
> Tested on x86-64/Linux, OK for all active branches?
> 
> 
> 2024-03-11  Eric Botcazou  <ebotcazou@adacore.com>
> 
> 	PR ipa/113996
> 	* ipa-icf.h (sem_function): Add static_chain_present member.
> 	* ipa-icf.cc (sem_function::get_hash): Hash it.
> 	(sem_function::equals_wpa): Compare it.
> 	(sem_function::equals_private): Likewise.
> 	(sem_function::init): Initialize it.
> 
> 
> 2024-03-11  Eric Botcazou  <ebotcazou@adacore.com>
> 
> 	* gnat.dg/lto27.adb: New test.
OK.
jeff

Re: [PATCH] Fix PR ipa/113996

[Jan Hubicka]

> 
> 
> On 3/11/24 4:38 PM, Eric Botcazou wrote:
> > Hi,
> > 
> > this is a regression present on all active branches: the attached Ada testcase
> > triggers an assertion failure when compiled with -O2 -gnatp -flto:
> > 
> >    /* Initialize the static chain.  */
> >    p = DECL_STRUCT_FUNCTION (fn)->static_chain_decl;
> >    gcc_assert (fn != current_function_decl);
> >    if (p)
> >      {
> >        /* No static chain?  Seems like a bug in tree-nested.cc.  */
> >        gcc_assert (static_chain);                                  <--- here
> > 
> >        setup_one_parameter (id, p, static_chain, fn, bb, &vars);
> >      }
> > 
> > The problem is that the ICF pass identifies two functions, one of which has a
> > static chain but the other does not.  The proposed fix is just to prevent this
> > identification from occurring.
> > 
> > Tested on x86-64/Linux, OK for all active branches?
> > 
> > 
> > 2024-03-11  Eric Botcazou  <ebotcazou@adacore.com>
> > 
> > 	PR ipa/113996
> > 	* ipa-icf.h (sem_function): Add static_chain_present member.
> > 	* ipa-icf.cc (sem_function::get_hash): Hash it.
> > 	(sem_function::equals_wpa): Compare it.
> > 	(sem_function::equals_private): Likewise.
> > 	(sem_function::init): Initialize it.
> > 
> > 
> > 2024-03-11  Eric Botcazou  <ebotcazou@adacore.com>
> > 
> > 	* gnat.dg/lto27.adb: New test.
> OK.
Patch is still OK, but ipa-ICF will only identify the functions if
static chain is unused. Perhaps just picking the winning candidate to be
version without static chain and making ipa-inline to not ICE when calls
with static chain lands to function with no static chain would help us
to optimize better.

Also IPA-SRA can optimize out unused static chains and ipa-prop can
propagate across them.

Honza
> jeff
> 

Re: [PATCH] Fix PR ipa/113996

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 648 bytes --]

> Patch is still OK, but ipa-ICF will only identify the functions if
> static chain is unused. Perhaps just picking the winning candidate to be
> version without static chain and making ipa-inline to not ICE when calls
> with static chain lands to function with no static chain would help us
> to optimize better.

I see, thanks for the explanation.  The attached patch appears to work.


	PR ipa/113996
	* ipa-icf.h (sem_function): Add static_chain_p member.
	* ipa-icf.cc (sem_function::init): Initialize it.
	(sem_item_optimizer::merge_classes): If the class is made of
	functions, pick one without static chain as the target.

-- 
Eric Botcazou

[-- Attachment #2: pr113996-2.diff --]
[-- Type: text/x-patch, Size: 2391 bytes --]

diff --git a/gcc/ipa-icf.cc b/gcc/ipa-icf.cc
index 5d5a42f9c6c..4fc02831798 100644
--- a/gcc/ipa-icf.cc
+++ b/gcc/ipa-icf.cc
@@ -1368,6 +1368,8 @@ sem_function::init (ipa_icf_gimple::func_checker *checker)
   /* iterating all function arguments.  */
   arg_count = count_formal_params (fndecl);
 
+  static_chain_p = func->static_chain_decl != NULL_TREE;
+
   edge_count = n_edges_for_fn (func);
   cgraph_node *cnode = dyn_cast <cgraph_node *> (node);
   if (!cnode->thunk)
@@ -3399,11 +3401,22 @@ sem_item_optimizer::merge_classes (unsigned int prev_class_count,
 
 	sem_item *source = c->members[0];
 
-	if (DECL_NAME (source->decl)
-	    && MAIN_NAME_P (DECL_NAME (source->decl)))
-	  /* If merge via wrappers, picking main as the target can be
-	     problematic.  */
-	  source = c->members[1];
+	if (source->type == FUNC)
+	  {
+	    /* Pick a member without static chain, if any.  */
+	    for (unsigned int j = 0; j < c->members.length (); j++)
+	      if (!static_cast<sem_function *> (c->members[j])->static_chain_p)
+		{
+		  source = c->members[j];
+		  break;
+		}
+
+	    /* If merge via wrappers, picking main as the target can be
+	       problematic.  */
+	    if (DECL_NAME (source->decl)
+		&& MAIN_NAME_P (DECL_NAME (source->decl)))
+	      source = c->members[source == c->members[0] ? 1 : 0];
+	  }
 
 	for (unsigned int j = 0; j < c->members.length (); j++)
 	  {
diff --git a/gcc/ipa-icf.h b/gcc/ipa-icf.h
index ef7e41bfa88..da20862c306 100644
--- a/gcc/ipa-icf.h
+++ b/gcc/ipa-icf.h
@@ -355,6 +355,9 @@ public:
      parameters.  */
   bool compatible_parm_types_p (tree, tree);
 
+  /* Return true if parameter I may be used.  */
+  bool param_used_p (unsigned int i);
+
   /* Exception handling region tree.  */
   eh_region region_tree;
 
@@ -379,6 +382,9 @@ public:
   /* Total number of SSA names used in the function.  */
   unsigned ssa_names_size;
 
+  /* Whether the special PARM_DECL for the static chain is present.  */
+  bool static_chain_p;
+
   /* Array of structures for all basic blocks.  */
   vec <ipa_icf_gimple::sem_bb *> bb_sorted;
 
@@ -386,9 +392,6 @@ public:
      function.  */
   hashval_t m_alias_sets_hash;
 
-  /* Return true if parameter I may be used.  */
-  bool param_used_p (unsigned int i);
-
 private:
   /* Calculates hash value based on a BASIC_BLOCK.  */
   hashval_t get_bb_hash (const ipa_icf_gimple::sem_bb *basic_block);

Re: [PATCH] Fix PR ipa/113996

[Jan Hubicka]

> > Patch is still OK, but ipa-ICF will only identify the functions if
> > static chain is unused. Perhaps just picking the winning candidate to be
> > version without static chain and making ipa-inline to not ICE when calls
> > with static chain lands to function with no static chain would help us
> > to optimize better.
> 
> I see, thanks for the explanation.  The attached patch appears to work.
> 
> 
> 	PR ipa/113996
> 	* ipa-icf.h (sem_function): Add static_chain_p member.
> 	* ipa-icf.cc (sem_function::init): Initialize it.
> 	(sem_item_optimizer::merge_classes): If the class is made of
> 	functions, pick one without static chain as the target.
> 
> -- 
> Eric Botcazou


> @@ -3399,11 +3401,22 @@ sem_item_optimizer::merge_classes (unsigned int prev_class_count,
>  
>  	sem_item *source = c->members[0];
>  
> -	if (DECL_NAME (source->decl)
> -	    && MAIN_NAME_P (DECL_NAME (source->decl)))
> -	  /* If merge via wrappers, picking main as the target can be
> -	     problematic.  */
> -	  source = c->members[1];
> +	if (source->type == FUNC)
> +	  {
> +	    /* Pick a member without static chain, if any.  */
> +	    for (unsigned int j = 0; j < c->members.length (); j++)
> +	      if (!static_cast<sem_function *> (c->members[j])->static_chain_p)
> +		{
> +		  source = c->members[j];
> +		  break;
> +		}
> +
> +	    /* If merge via wrappers, picking main as the target can be
> +	       problematic.  */
> +	    if (DECL_NAME (source->decl)
> +		&& MAIN_NAME_P (DECL_NAME (source->decl)))
> +	      source = c->members[source == c->members[0] ? 1 : 0];

Thanks for looking into this.
I wonder if it can happen that we ICF merge function with static chain
and main?
We can fix this unlikely case by simply considering functions with
static chain not equivalent to MAIN_NAME_P function.

On x86_64 static chain goes to R10, but I wonder if we support targets
where API of function taking static chain and not using it differs from
API of function with no static chain?

Honza