Bug 63580

You miss to mark p1 addressable in the alias decl (that is, copy TREE_ADDRESSABLE).

(In reply to Richard Biener from comment #1)
> You miss to mark p1 addressable in the alias decl (that is, copy
> TREE_ADDRESSABLE).

As I can see, ICF creates thunk to a localalias of foo.
At which please I should preserve addressable attribute?

Thanks,
Martin

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63580
>
> Richard Biener <rguenth at gcc dot gnu.org> changed:
>
>             What    |Removed                     |Added
> ----------------------------------------------------------------------------
>               Status|UNCONFIRMED                 |NEW
>     Last reconfirmed|                            |2014-10-20
>       Ever confirmed|0                           |1
>
> --- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
> You miss to mark p1 addressable in the alias decl (that is, copy
> TREE_ADDRESSABLE).
>

Do you mean following change:

@@ -2334,6 +2334,14 @@ cgraph_node::create_wrapper (cgraph_node *target)

      cgraph_edge *e = create_edge (target, NULL, 0, CGRAPH_FREQ_BASE);

+  tree arguments = DECL_ARGUMENTS (decl);
+
+  while (arguments)
+    {
+      TREE_ADDRESSABLE (arguments) = false;
+      arguments = TREE_CHAIN (arguments);
+    }
+
      expand_thunk (false, true);
      e->call_stmt_cannot_inline_p = true;

Thanks,
Martin

diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 6f61f5c..89c96dc 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -2342,6 +2342,14 @@ cgraph_node::create_wrapper (cgraph_node *target)
 
     cgraph_edge *e = create_edge (target, NULL, 0, CGRAPH_FREQ_BASE);
 
+    tree arguments = DECL_ARGUMENTS (decl);
+
+    while (arguments)
+      {
+       TREE_ADDRESSABLE (arguments) = false;
+       arguments = TREE_CHAIN (arguments);
+      }
+
     expand_thunk (false, true);
     e->call_stmt_cannot_inline_p = true;

Following patch fixed the issue, boostrap works without any regression.
But I guess it's just a partial fix. There's very similar issue: PR63573.

Thanks,
Martin

Hmm, I think we may get confused by having ADDRESSABLE flag set because the original function body took its address and it makes sense to clear it as Martin suggest.
I am bit confused on why this leads to ICE - I think false positive on ADDRESSABLE flag ought to make cgraphunit to introduce extra copy in:

      if (nargs)
        for (i = 1, arg = DECL_CHAIN (a); i < nargs; i++, arg = DECL_CHAIN (arg))
          {
            tree tmp = arg;
            if (!is_gimple_val (arg))
              {
                tmp = create_tmp_reg (TYPE_MAIN_VARIANT
                                      (TREE_TYPE (arg)), "arg");
                gimple stmt = gimple_build_assign (tmp, arg);
                gsi_insert_after (&bsi, stmt, GSI_NEW_STMT);
              }
            vargs.quick_push (tmp);
          }

that may later end up on stack and prevent call from being a tail call.

I see that later optimizers may not be able to optimize out the extra copy simply because it is bit hard to do: one needs to verify that the original location is dead and possibly may be rewritten by a callee.

I did not find any code that would prevent clearning ADDRESSABLE flag on parameters, so I think Martin's patch is correct.

Additionally I think it is not safe to set TAILCALL flag if the code above introduced a local variable. This is becuase setting this flag by hand bypasses logic in suitable_for_tail_call_opt_p that prevents tail call conversion when any ADDRESSABLE vars exists.

I did not find any reasons why ADDRESSABLE flag should be set on parameter besides the usual case that it has address taken, so I tink Martin's patch is OK and correct fix.

Martin, does it fix the other PR too?

*** Bug 63721 has been marked as a duplicate of this bug. ***

Author: marxin
Date: Fri Nov  7 13:37:41 2014
New Revision: 217222

URL: https://gcc.gnu.org/viewcvs?rev=217222&root=gcc&view=rev
Log:
	PR ipa/63580
        * cgraphunit.c (cgraph_node::create_wrapper):
	TREE_ADDRESSABLE is set to false for a newly created thunk.
	* g++.dg/ipa/pr63580.C: New test.


Added:
    trunk/gcc/testsuite/g++.dg/ipa/pr63580.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cgraphunit.c
    trunk/gcc/testsuite/ChangeLog

The original testcase still ICEs on ppc64.

(In reply to Markus Trippelsdorf from comment #8)
> The original testcase still ICEs on ppc64.

Sorry, it actually works fine now.