diff mbox

Fix thunk expansion (PR ipa/64896)

Message ID CAD57uCfDKomop66KRd9RGhwH6aX=mKnJqjpVm_LHfyoTbsDCkw@mail.gmail.com
State New
Headers show

Commit Message

Yvan Roux March 9, 2015, 4:07 p.m. UTC
Hi,

As added in the PR, this issue is also present on 4.9 branch and
affects at least arm-linux-gnueabihf target (as reported in PR61207).

I've backported it in the 4.9 branch with the attached patch.  The
difference with the trunk code is due the code introduced by PR63587
fix (I didn't checked on power7, on which the PR was initially
reported, but I didn't managed to reproduce the issue for arm targets
on 4.9 branch).

Boostrapped on x86_64, and tested on arm/aarch64 targets (regression
testing is ongoing). is ok for 4.9 branch when  validation is done ?


Thanks
Yvan

gcc/
2015-03-09  Yvan Roux  <yvan.roux@linaro.org>

    Backport from trunk r220489.
    2015-02-06  Jakub Jelinek  <jakub@redhat.com>

    PR ipa/64896
    * cgraphunit.c (cgraph_node::expand_thunk): If
    restype is not is_gimple_reg_type nor the thunk_fndecl
    returns aggregate_value_p, set restmp to a temporary variable
    instead of resdecl.

gcc/testsuite/
2015-03-09  Yvan Roux  <yvan.roux@linaro.org>

    Backport from trunk r220489.
    2015-02-06  Jakub Jelinek  <jakub@redhat.com>

    PR ipa/64896
    * g++.dg/ipa/pr64896.C: New test.

Comments

Yvan Roux March 10, 2015, 5:35 p.m. UTC | #1
Hi

On 9 March 2015 at 17:07, Yvan Roux <yvan.roux@linaro.org> wrote:
> Hi,
>
> As added in the PR, this issue is also present on 4.9 branch and
> affects at least arm-linux-gnueabihf target (as reported in PR61207).
>
> I've backported it in the 4.9 branch with the attached patch.  The
> difference with the trunk code is due the code introduced by PR63587
> fix (I didn't checked on power7, on which the PR was initially
> reported, but I didn't managed to reproduce the issue for arm targets
> on 4.9 branch).
>
> Boostrapped on x86_64, and tested on arm/aarch64 targets (regression
> testing is ongoing). is ok for 4.9 branch when  validation is done ?

So bootstrapped/regtested on x86_64 and cross-compiled/regtested on
aarch64-linux-gnu
arm-linux-gnueabihf
armeb-linux-gnueabihf
i686-linux-gnu

> Thanks
> Yvan
>
> gcc/
> 2015-03-09  Yvan Roux  <yvan.roux@linaro.org>
>
>     Backport from trunk r220489.
>     2015-02-06  Jakub Jelinek  <jakub@redhat.com>
>
>     PR ipa/64896
>     * cgraphunit.c (cgraph_node::expand_thunk): If
>     restype is not is_gimple_reg_type nor the thunk_fndecl
>     returns aggregate_value_p, set restmp to a temporary variable
>     instead of resdecl.
>
> gcc/testsuite/
> 2015-03-09  Yvan Roux  <yvan.roux@linaro.org>
>
>     Backport from trunk r220489.
>     2015-02-06  Jakub Jelinek  <jakub@redhat.com>
>
>     PR ipa/64896
>     * g++.dg/ipa/pr64896.C: New test.
Yvan Roux March 10, 2015, 7:09 p.m. UTC | #2
On 10 March 2015 at 19:18, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Hi
>>
>> On 9 March 2015 at 17:07, Yvan Roux <yvan.roux@linaro.org> wrote:
>> > Hi,
>> >
>> > As added in the PR, this issue is also present on 4.9 branch and
>> > affects at least arm-linux-gnueabihf target (as reported in PR61207).
>> >
>> > I've backported it in the 4.9 branch with the attached patch.  The
>> > difference with the trunk code is due the code introduced by PR63587
>> > fix (I didn't checked on power7, on which the PR was initially
>> > reported, but I didn't managed to reproduce the issue for arm targets
>> > on 4.9 branch).
>> >
>> > Boostrapped on x86_64, and tested on arm/aarch64 targets (regression
>> > testing is ongoing). is ok for 4.9 branch when  validation is done ?
>>
>> So bootstrapped/regtested on x86_64 and cross-compiled/regtested on
>> aarch64-linux-gnu
>> arm-linux-gnueabihf
>> armeb-linux-gnueabihf
>> i686-linux-gnu
>
> This is OK. note that cgraph_node::expand_thunk has gathered quite few
> extra fixes that may be resonable for backporting.  Looking across
> changes after ipa-icf was enabled I think we should look into
> these:
>
>         PR ipa/65236
>         * cgraphunit.c (cgraph_node::expand_thunk): Enable return slot
>         opt.
>
>         PR ipa/64813
>         * cgraphunit.c (cgraph_node::expand_thunk): Do not create
>         a return value for call to a function that is noreturn.
>
>         PR ipa/63595
>         * cgraphunit.c (cgraph_node::expand_thunk): DECL_BY_REFERENCE
>         is correctly handled for thunks created by IPA ICF.
>
>         PR ipa/63587
>         * cgraphunit.c (cgraph_node::expand_thunk): Only VAR_DECLs are put
>         to local declarations.
>         * function.c (add_local_decl): Implementation moved from header
>         file, assert introduced for tree type.
>         * function.h: Likewise.
>
> While these bugs was triggered by ipa-icf, they all IMO can be reproduced by
> thunks on targets that do not define assembler thunks.
> (most are about return values and those are not excercised on main targets with
> MI thunks because covariant thunks always returns pointer)

Thanks Honza.  I can backport all of them and pass the same validation
I did for this one if you want.

Yvan
Yvan Roux March 10, 2015, 11 p.m. UTC | #3
Honza,

On 10 March 2015 at 20:09, Yvan Roux <yvan.roux@linaro.org> wrote:
> On 10 March 2015 at 19:18, Jan Hubicka <hubicka@ucw.cz> wrote:
>>> Hi
>>>
>>> On 9 March 2015 at 17:07, Yvan Roux <yvan.roux@linaro.org> wrote:
>>> > Hi,
>>> >
>>> > As added in the PR, this issue is also present on 4.9 branch and
>>> > affects at least arm-linux-gnueabihf target (as reported in PR61207).
>>> >
>>> > I've backported it in the 4.9 branch with the attached patch.  The
>>> > difference with the trunk code is due the code introduced by PR63587
>>> > fix (I didn't checked on power7, on which the PR was initially
>>> > reported, but I didn't managed to reproduce the issue for arm targets
>>> > on 4.9 branch).
>>> >
>>> > Boostrapped on x86_64, and tested on arm/aarch64 targets (regression
>>> > testing is ongoing). is ok for 4.9 branch when  validation is done ?
>>>
>>> So bootstrapped/regtested on x86_64 and cross-compiled/regtested on
>>> aarch64-linux-gnu
>>> arm-linux-gnueabihf
>>> armeb-linux-gnueabihf
>>> i686-linux-gnu
>>
>> This is OK. note that cgraph_node::expand_thunk has gathered quite few
>> extra fixes that may be resonable for backporting.  Looking across
>> changes after ipa-icf was enabled I think we should look into
>> these:
>>
>>         PR ipa/65236
>>         * cgraphunit.c (cgraph_node::expand_thunk): Enable return slot
>>         opt.

This bugfix adds ipa-icf-6.C test which failed on 4.9 branch as
ipa-icf is not backported on that branch.  Is the bugfix still
relevant and we can dropped the testcase ?

>>         PR ipa/64813
>>         * cgraphunit.c (cgraph_node::expand_thunk): Do not create
>>         a return value for call to a function that is noreturn.
>>
>>         PR ipa/63595
>>         * cgraphunit.c (cgraph_node::expand_thunk): DECL_BY_REFERENCE
>>         is correctly handled for thunks created by IPA ICF.
>>
>>         PR ipa/63587
>>         * cgraphunit.c (cgraph_node::expand_thunk): Only VAR_DECLs are put
>>         to local declarations.
>>         * function.c (add_local_decl): Implementation moved from header
>>         file, assert introduced for tree type.
>>         * function.h: Likewise.
>>
>> While these bugs was triggered by ipa-icf, they all IMO can be reproduced by
>> thunks on targets that do not define assembler thunks.
>> (most are about return values and those are not excercised on main targets with
>> MI thunks because covariant thunks always returns pointer)
>
> Thanks Honza.  I can backport all of them and pass the same validation
> I did for this one if you want.

The test introduced


> Yvan
diff mbox

Patch

diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 8f57607..130fc0d 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -1572,9 +1572,14 @@  expand_thunk (struct cgraph_node *node, bool output_asm_thunks)
 	    restmp = gimple_fold_indirect_ref (resdecl);
 	  else if (!is_gimple_reg_type (restype))
 	    {
-	      restmp = resdecl;
-	      add_local_decl (cfun, restmp);
-	      BLOCK_VARS (DECL_INITIAL (current_function_decl)) = restmp;
+	      if (aggregate_value_p (resdecl, TREE_TYPE (thunk_fndecl)))
+		{
+		  restmp = resdecl;
+		  add_local_decl (cfun, restmp);
+		  BLOCK_VARS (DECL_INITIAL (current_function_decl)) = restmp;
+		}
+	      else
+		restmp = create_tmp_var (restype, "retval");
 	    }
 	  else
 	    restmp = create_tmp_reg (restype, "retval");
diff --git a/gcc/testsuite/g++.dg/ipa/pr64896.C b/gcc/testsuite/g++.dg/ipa/pr64896.C
new file mode 100644
index 0000000..0a78220
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/pr64896.C
@@ -0,0 +1,29 @@ 
+// PR ipa/64896
+// { dg-do compile }
+// { dg-options "-O2" }
+
+struct A { int a, b; };
+struct B { A c; int d; };
+struct C { virtual B fn1 () const; };
+struct D { B fn2 () const; int fn3 () const; C *fn4 () const; };
+
+int
+D::fn3 () const
+{
+  fn4 ()->fn1 ();
+}
+
+B
+D::fn2 () const
+{
+  return B ();
+}
+
+class F : C
+{
+  B
+  fn1 () const
+  {
+    return B ();
+  }
+};