Re: [PATCH] Fix thunk expansion (PR ipa/64896)
Ping. On 11 March 2015 at 16:38, Yvan Roux yvan.r...@linaro.org wrote: Hi, 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. Here is the two patches that backport PR ipa/63587 and PR ipa/64813 fixes in 4.9 branch. The 2 others introduce test cases that check ipa-icf pass dumps, so I'm not sure if the code has to be backported. bootstrapped/regtested on x86_64 and cross-compiled/regtested on aarch64-linux-gnu arm-linux-gnueabihf armeb-linux-gnueabihf i686-linux-gnu Ok for 4.9 ? Thanks Yvan - PR 63587 - gcc/ 2015-03-11 Yvan Roux yvan.r...@linaro.org Backport from trunk r216841. 2014-10-29 Martin Liska mli...@suse.cz 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. gcc/testsuite/ 2015-03-11 Yvan Roux yvan.r...@linaro.org Backport from trunk r216841. 2014-10-29 Martin Liska mli...@suse.cz PR ipa/63587 * g++.dg/ipa/pr63587-1.C: New test. * g++.dg/ipa/pr63587-2.C: New test. - PR 64813 - 2015-03-11 Yvan Roux yvan.r...@linaro.org Backport from trunk r220616. 2015-02-11 Martin Liska mli...@suse.cz PR ipa/64813 * cgraphunit.c (cgraph_node::expand_thunk): Do not create a return value for call to a function that is noreturn.
Re: [PATCH] Fix thunk expansion (PR ipa/64896)
gcc/ 2015-03-11 Yvan Roux yvan.r...@linaro.org Backport from trunk r216841. 2014-10-29 Martin Liska mli...@suse.cz 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. gcc/testsuite/ 2015-03-11 Yvan Roux yvan.r...@linaro.org Backport from trunk r216841. 2014-10-29 Martin Liska mli...@suse.cz PR ipa/63587 * g++.dg/ipa/pr63587-1.C: New test. * g++.dg/ipa/pr63587-2.C: New test. - PR 64813 - 2015-03-11 Yvan Roux yvan.r...@linaro.org Backport from trunk r220616. 2015-02-11 Martin Liska mli...@suse.cz PR ipa/64813 * cgraphunit.c (cgraph_node::expand_thunk): Do not create a return value for call to a function that is noreturn. OK, thanks! Honza
Re: [PATCH] Fix thunk expansion (PR ipa/64896)
Hi,
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.
Here is the two patches that backport PR ipa/63587 and PR ipa/64813
fixes in 4.9 branch. The 2 others introduce test cases that check
ipa-icf pass dumps, so I'm not sure if the code has to be backported.
bootstrapped/regtested on x86_64 and cross-compiled/regtested on
aarch64-linux-gnu
arm-linux-gnueabihf
armeb-linux-gnueabihf
i686-linux-gnu
Ok for 4.9 ?
Thanks
Yvan
- PR 63587 -
gcc/
2015-03-11 Yvan Roux yvan.r...@linaro.org
Backport from trunk r216841.
2014-10-29 Martin Liska mli...@suse.cz
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.
gcc/testsuite/
2015-03-11 Yvan Roux yvan.r...@linaro.org
Backport from trunk r216841.
2014-10-29 Martin Liska mli...@suse.cz
PR ipa/63587
* g++.dg/ipa/pr63587-1.C: New test.
* g++.dg/ipa/pr63587-2.C: New test.
- PR 64813 -
2015-03-11 Yvan Roux yvan.r...@linaro.org
Backport from trunk r220616.
2015-02-11 Martin Liska mli...@suse.cz
PR ipa/64813
* cgraphunit.c (cgraph_node::expand_thunk): Do not create
a return value for call to a function that is noreturn.
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 130fc0d..27016ad 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -1575,7 +1575,9 @@ expand_thunk (struct cgraph_node *node, bool output_asm_thunks)
if (aggregate_value_p (resdecl, TREE_TYPE (thunk_fndecl)))
{
restmp = resdecl;
- add_local_decl (cfun, restmp);
+
+ if (TREE_CODE (restmp) == VAR_DECL)
+ add_local_decl (cfun, restmp);
BLOCK_VARS (DECL_INITIAL (current_function_decl)) = restmp;
}
else
diff --git a/gcc/function.c b/gcc/function.c
index 1a8682b..b377667 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -7193,6 +7193,15 @@ match_asm_constraints_1 (rtx insn, rtx *p_sets, int noutputs)
df_insn_rescan (insn);
}
+/* Add the decl D to the local_decls list of FUN. */
+
+void
+add_local_decl (struct function *fun, tree d)
+{
+ gcc_assert (TREE_CODE (d) == VAR_DECL);
+ vec_safe_push (fun-local_decls, d);
+}
+
static unsigned
rest_of_match_asm_constraints (void)
{
diff --git a/gcc/function.h b/gcc/function.h
index 38a0fc4..fd4639c 100644
--- a/gcc/function.h
+++ b/gcc/function.h
@@ -674,11 +674,7 @@ struct GTY(()) function {
/* Add the decl D to the local_decls list of FUN. */
-static inline void
-add_local_decl (struct function *fun, tree d)
-{
- vec_safe_push (fun-local_decls, d);
-}
+void add_local_decl (struct function *fun, tree d);
#define FOR_EACH_LOCAL_DECL(FUN, I, D) \
FOR_EACH_VEC_SAFE_ELT_REVERSE ((FUN)-local_decls, I, D)
diff --git a/gcc/testsuite/g++.dg/ipa/pr63587-1.C b/gcc/testsuite/g++.dg/ipa/pr63587-1.C
new file mode 100644
index 000..cbf872e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/pr63587-1.C
@@ -0,0 +1,92 @@
+// PR ipa/63587
+// { dg-do compile { target c++11 } }
+// { dg-options -O2 -fno-strict-aliasing }
+
+template class struct A
+{
+};
+template typename struct B
+{
+ template typename struct C;
+};
+class D;
+template typename class F;
+struct G
+{
+ void operator()(const D , D);
+};
+class D
+{
+public:
+ D (int);
+};
+struct H
+{
+ H (int);
+};
+template typename _Key, typename, typename, typename _Compare, typename
+class I
+{
+ typedef _Key key_type;
+ template typename _Key_compare struct J
+ {
+_Key_compare _M_key_compare;
+ };
+ J_Compare _M_impl;
+
+public:
+ Aint _M_get_insert_unique_pos (const key_type );
+ Aint _M_get_insert_hint_unique_pos (H );
+ template typename... _Args int _M_emplace_hint_unique (H, _Args ...);
+};
+template typename _Key, typename _Tp, typename _Compare = G,
+ typename _Alloc = FA_Tp
+class K
+{
+ typedef _Key key_type;
+ typedef _Key value_type;
+ typedef typename B_Alloc::template Cvalue_type _Pair_alloc_type;
+ Ikey_type, value_type, int, _Compare, _Pair_alloc_type _M_t;
+
+public:
+ void operator[](key_type)
+ {
+
Re: [PATCH] Fix thunk expansion (PR ipa/64896)
Hi On 9 March 2015 at 17:07, Yvan Roux yvan.r...@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.r...@linaro.org Backport from trunk r220489. 2015-02-06 Jakub Jelinek ja...@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.r...@linaro.org Backport from trunk r220489. 2015-02-06 Jakub Jelinek ja...@redhat.com PR ipa/64896 * g++.dg/ipa/pr64896.C: New test.
Re: [PATCH] Fix thunk expansion (PR ipa/64896)
Hi On 9 March 2015 at 17:07, Yvan Roux yvan.r...@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
Re: [PATCH] Fix thunk expansion (PR ipa/64896)
On 10 March 2015 at 19:18, Jan Hubicka hubi...@ucw.cz wrote: Hi On 9 March 2015 at 17:07, Yvan Roux yvan.r...@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
Re: [PATCH] Fix thunk expansion (PR ipa/64896)
Honza, On 10 March 2015 at 20:09, Yvan Roux yvan.r...@linaro.org wrote: On 10 March 2015 at 19:18, Jan Hubicka hubi...@ucw.cz wrote: Hi On 9 March 2015 at 17:07, Yvan Roux yvan.r...@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
Re: [PATCH] Fix thunk expansion (PR ipa/64896)
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.r...@linaro.org
Backport from trunk r220489.
2015-02-06 Jakub Jelinek ja...@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.r...@linaro.org
Backport from trunk r220489.
2015-02-06 Jakub Jelinek ja...@redhat.com
PR ipa/64896
* g++.dg/ipa/pr64896.C: New test.
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 000..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 ();
+ }
+};
Re: [PATCH] Fix thunk expansion (PR ipa/64896)
Hi!
As discussed in the PR, for functions that return an aggregate that is not
aggregate_value_p (i.e. returned in registers), using RESULT_DECL is
undesirable, that's not what we normally emit for user code.
So, this patch instead uses a temporary, which is optimized right now as
much as similar user written code.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK,
thanks!
Honza
2015-02-06 Jakub Jelinek ja...@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.
* g++.dg/ipa/pr64896.C: New test.
--- gcc/cgraphunit.c.jj 2015-01-29 21:38:21.0 +0100
+++ gcc/cgraphunit.c 2015-02-06 13:18:44.870527405 +0100
@@ -1609,11 +1609,16 @@ cgraph_node::expand_thunk (bool output_a
}
else if (!is_gimple_reg_type (restype))
{
- restmp = resdecl;
+ if (aggregate_value_p (resdecl, TREE_TYPE (thunk_fndecl)))
+ {
+ restmp = resdecl;
- if (TREE_CODE (restmp) == VAR_DECL)
- add_local_decl (cfun, restmp);
- BLOCK_VARS (DECL_INITIAL (current_function_decl)) = restmp;
+ if (TREE_CODE (restmp) == VAR_DECL)
+ 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);
--- gcc/testsuite/g++.dg/ipa/pr64896.C.jj 2015-02-06 13:36:30.076680258
+0100
+++ gcc/testsuite/g++.dg/ipa/pr64896.C2015-02-06 13:36:13.0
+0100
@@ -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 ();
+ }
+};
Jakub
