public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] Fix PR ipa/113996
@ 2024-03-11 22:38 Eric Botcazou
  2024-03-12  0:06 ` Jeff Law
  0 siblings, 1 reply; 5+ messages in thread
From: Eric Botcazou @ 2024-03-11 22:38 UTC (permalink / raw)
  To: gcc-patches

[-- 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);

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Fix PR ipa/113996
  2024-03-11 22:38 [PATCH] Fix PR ipa/113996 Eric Botcazou
@ 2024-03-12  0:06 ` Jeff Law
  2024-03-12 13:24   ` Jan Hubicka
  0 siblings, 1 reply; 5+ messages in thread
From: Jeff Law @ 2024-03-12  0:06 UTC (permalink / raw)
  To: Eric Botcazou, gcc-patches



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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Fix PR ipa/113996
  2024-03-12  0:06 ` Jeff Law
@ 2024-03-12 13:24   ` Jan Hubicka
  2024-03-12 17:48     ` Eric Botcazou
  0 siblings, 1 reply; 5+ messages in thread
From: Jan Hubicka @ 2024-03-12 13:24 UTC (permalink / raw)
  To: Jeff Law, mjambor; +Cc: Eric Botcazou, gcc-patches

> 
> 
> 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
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Fix PR ipa/113996
  2024-03-12 13:24   ` Jan Hubicka
@ 2024-03-12 17:48     ` Eric Botcazou
  2024-03-14 16:52       ` Jan Hubicka
  0 siblings, 1 reply; 5+ messages in thread
From: Eric Botcazou @ 2024-03-12 17:48 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: Jeff Law, mjambor, gcc-patches

[-- 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);

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Fix PR ipa/113996
  2024-03-12 17:48     ` Eric Botcazou
@ 2024-03-14 16:52       ` Jan Hubicka
  0 siblings, 0 replies; 5+ messages in thread
From: Jan Hubicka @ 2024-03-14 16:52 UTC (permalink / raw)
  To: Eric Botcazou; +Cc: Jeff Law, mjambor, gcc-patches

> > 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-03-14 16:52 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-11 22:38 [PATCH] Fix PR ipa/113996 Eric Botcazou
2024-03-12  0:06 ` Jeff Law
2024-03-12 13:24   ` Jan Hubicka
2024-03-12 17:48     ` Eric Botcazou
2024-03-14 16:52       ` Jan Hubicka

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).