| Message ID | CAD57uCfDKomop66KRd9RGhwH6aX=mKnJqjpVm_LHfyoTbsDCkw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
> 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) Honza
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
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 --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 (); + } +};
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.