[PATCH] Fix ICF sem_function::merge (PR target/63892)

[PATCH] Fix ICF sem_function::merge (PR target/63892)

[Jakub Jelinek]

Hi!

As written in the PR, the sibcall-3.c testcase fails on Darwin, because
!sem_item::target_supports_symbol_aliases_p () and we don't really try
redirect_callers, even when that is the best way to perform ICF (both
original and alias are local).

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2015-02-20  Jakub Jelinek  <jakub@redhat.com>
	    Martin Liska  <mliska@suse.cz>

	PR target/63892
	* ipa-icf.c (sem_function::merge): If DECL_COMDAT_GROUP (alias->decl),
	don't try to create_thunk if stdarg_p.  If
	!sem_item::target_supports_symbol_aliases_p (), similarly, and try to
	redirect_callers if possible.
	(sem_item_optimizer::execute): Call unregister_hooks here...
	(ipa_icf_driver): ... instead of here.

--- gcc/ipa-icf.c
+++ gcc/ipa-icf.c
@@ -651,7 +651,9 @@ sem_function::merge (sem_item *alias_item)
      section (or we risk link failures when section is discarded).  */
   if ((original_address_matters
        && alias_address_matters)
-      || original_discardable)
+      || original_discardable
+      || DECL_COMDAT_GROUP (alias->decl)
+      || !sem_item::target_supports_symbol_aliases_p ())
     {
       create_thunk = !stdarg_p (TREE_TYPE (alias->decl));
       create_alias = false;
@@ -659,6 +661,7 @@ sem_function::merge (sem_item *alias_item)
          the extra thunk wrapper for direct calls.  */
       redirect_callers
 	= (!original_discardable
+	   && !DECL_COMDAT_GROUP (alias->decl)
 	   && alias->get_availability () > AVAIL_INTERPOSABLE
 	   && original->get_availability () > AVAIL_INTERPOSABLE
 	   && !alias->instrumented_version);
@@ -670,13 +673,6 @@ sem_function::merge (sem_item *alias_item)
       redirect_callers = false;
     }
 
-  if (create_alias && (DECL_COMDAT_GROUP (alias->decl)
-		       || !sem_item::target_supports_symbol_aliases_p ()))
-    {
-      create_alias = false;
-      create_thunk = true;
-    }
-
   /* We want thunk to always jump to the local function body
      unless the body is comdat and may be optimized out.  */
   if ((create_thunk || redirect_callers)
@@ -1717,6 +1713,8 @@ void
 sem_item_optimizer::execute (void)
 {
   filter_removed_items ();
+  unregister_hooks ();
+
   build_hash_based_classes ();
 
   if (dump_file)
@@ -2578,7 +2576,6 @@ ipa_icf_driver (void)
   gcc_assert (optimizer);
 
   optimizer->execute ();
-  optimizer->unregister_hooks ();
 
   delete optimizer;
   optimizer = NULL;

	Jakub

Re: [PATCH] Fix ICF sem_function::merge (PR target/63892)

[Jeff Law]

On 02/20/15 07:48, Jakub Jelinek wrote:
> Hi!
>
> As written in the PR, the sibcall-3.c testcase fails on Darwin, because
> !sem_item::target_supports_symbol_aliases_p () and we don't really try
> redirect_callers, even when that is the best way to perform ICF (both
> original and alias are local).
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK.

jeff

Re: [PATCH] Fix ICF sem_function::merge (PR target/63892)

[Iain Sandoe]

Hi Jakub, Martin,

On 20 Feb 2015, at 16:42, Jeff Law wrote:

> On 02/20/15 07:48, Jakub Jelinek wrote:
>> Hi!
>> 
>> As written in the PR, the sibcall-3.c testcase fails on Darwin, because
>> !sem_item::target_supports_symbol_aliases_p () and we don't really try
>> redirect_callers, even when that is the best way to perform ICF (both
>> original and alias are local).
>> 
>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> OK.

This caused a bootstrap fail on Darwin, because of what looks like a typo in the re-factoring of the code (unfortunately, doesn't show up until stage#2).

I've bootstrapped the following patch on x86_64-linux and x86_64-darwin12, OK for trunk if wider testing passes?

Iain

P.S. The patch does solve a problem with ADT/SmallVectorTests.cpp in llvm suite (with generation of a varargs thunk).
However, it does not appear to restore sibcall-3 for m32 darwin (see. pr63892 for updated analysis).

gcc 
	* ipa-icf.c (sem_function::merge): Do not try to redirect unless the target supports
	symbol aliases.

diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index e1af8bf..f128494 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -662,6 +662,7 @@ sem_function::merge (sem_item *alias_item)
       redirect_callers
 	= (!original_discardable
 	   && !DECL_COMDAT_GROUP (alias->decl)
+	   && sem_item::target_supports_symbol_aliases_p ()
 	   && alias->get_availability () > AVAIL_INTERPOSABLE
 	   && original->get_availability () > AVAIL_INTERPOSABLE
 	   && !alias->instrumented_version);

Re: [PATCH] Fix ICF sem_function::merge (PR target/63892)

[Jakub Jelinek]

On Sat, Feb 21, 2015 at 01:24:55PM +0000, Iain Sandoe wrote:
> P.S. The patch does solve a problem with ADT/SmallVectorTests.cpp in llvm suite (with generation of a varargs thunk).
> However, it does not appear to restore sibcall-3 for m32 darwin (see. pr63892 for updated analysis).
> 
> gcc 
> 	* ipa-icf.c (sem_function::merge): Do not try to redirect unless the target supports
> 	symbol aliases.

No, that looks wrong.  It is very much intentional, there should be no
reason why even without proper support of aliases you couldn't redirect
callers.  Callers redirection is done by just changing the IL, there is
really no need for any backend support for that.
Just look at the sibcall-3.c testcase.

> diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
> index e1af8bf..f128494 100644
> --- a/gcc/ipa-icf.c
> +++ b/gcc/ipa-icf.c
> @@ -662,6 +662,7 @@ sem_function::merge (sem_item *alias_item)
>        redirect_callers
>  	= (!original_discardable
>  	   && !DECL_COMDAT_GROUP (alias->decl)
> +	   && sem_item::target_supports_symbol_aliases_p ()
>  	   && alias->get_availability () > AVAIL_INTERPOSABLE
>  	   && original->get_availability () > AVAIL_INTERPOSABLE
>  	   && !alias->instrumented_version);

	Jakub