| Message ID | 20140103083121.GT892@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
Jakub Jelinek <jakub@redhat.com> wrote: >On Tue, Dec 31, 2013 at 11:30:12AM +0100, Richard Biener wrote: >> Meanwhile your patch is ok. > >As discussed in the PR, the patch wasn't sufficient, __cxa_pure_virtual >can appear in the vtable and it has pretty much the same properties >as __builtin_unreachable (void return value, no arguments, noreturn, >DCE or cfg cleanup being able to remove anything after it because of >noreturn). > >Additionally, the patch attempts to punt on invalid type changes >(ODR violations?, apparently none appear during bootstrap/regtest as >verified by additional logging added there) or inplace changes of >gimple_call_flags that would require some cleanups caller isn't >expected to >do (again, doesn't happen during bootstrap/regtest). > >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? I'd rather do nothing in the !inplace case, it's rare enough to not care. Also there is nothing wrong with wrong types et al - we have a perfect mechanism for dealing with this. Change the fndecl but not the fntype. I'd like to see the code simplified with keeping the above in mind. Thanks, Richard. >2014-01-03 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/59622 > * gimple-fold.c: Include calls.h. > (gimple_fold_call): Fix a typo in message. Handle > __cxa_pure_virtual similarly to __builtin_unreachable. For > inplace punt if gimple_call_flags would change. Verify > that lhs and argument types are compatible. > > * g++.dg/opt/pr59622-2.C: New test. > >--- gcc/gimple-fold.c.jj 2013-12-31 12:56:59.000000000 +0100 >+++ gcc/gimple-fold.c 2014-01-02 18:33:51.207921734 +0100 >@@ -51,6 +51,7 @@ along with GCC; see the file COPYING3. > #include "gimple-pretty-print.h" > #include "tree-ssa-address.h" > #include "langhooks.h" >+#include "calls.h" > > /* Return true when DECL can be referenced from current unit. >FROM_DECL (if non-null) specify constructor of variable DECL was taken >from. >@@ -1167,7 +1168,7 @@ gimple_fold_call (gimple_stmt_iterator * > (OBJ_TYPE_REF_EXPR (callee))))) > { > fprintf (dump_file, >- "Type inheritnace inconsistent devirtualization of "); >+ "Type inheritance inconsistent devirtualization of "); > print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM); > fprintf (dump_file, " to "); > print_generic_expr (dump_file, callee, TDF_SLIM); >@@ -1184,18 +1185,74 @@ gimple_fold_call (gimple_stmt_iterator * > = possible_polymorphic_call_targets (callee, &final); > if (final && targets.length () <= 1) > { >+ tree fndecl; >+ int call_flags = gimple_call_flags (stmt), new_flags; > if (targets.length () == 1) >+ fndecl = targets[0]->decl; >+ else >+ fndecl = builtin_decl_implicit (BUILT_IN_UNREACHABLE); >+ new_flags = flags_from_decl_or_type (fndecl); >+ if (inplace) > { >- gimple_call_set_fndecl (stmt, targets[0]->decl); >- changed = true; >+ /* For inplace, avoid changes of call flags that >+ might require cfg cleanups, EH cleanups, removal >+ of vdef/vuses etc. */ >+ if ((call_flags ^ new_flags) >+ & (ECF_NORETURN | ECF_NOTHROW | ECF_NOVOPS)) >+ final = false; >+ else if ((call_flags & ECF_NOVOPS) == 0) >+ { >+ if ((call_flags ^ new_flags) & ECF_CONST) >+ final = false; >+ else if (((call_flags & (ECF_PURE | ECF_CONST >+ | ECF_NORETURN)) == 0) >+ ^ ((new_flags & (ECF_PURE | ECF_CONST >+ | ECF_NORETURN)) == 0)) >+ final = false; >+ } > } >- else if (!inplace) >+ if (final) > { >- tree fndecl = builtin_decl_implicit (BUILT_IN_UNREACHABLE); >- gimple new_stmt = gimple_build_call (fndecl, 0); >- gimple_set_location (new_stmt, gimple_location (stmt)); >- gsi_insert_before (gsi, new_stmt, GSI_SAME_STMT); >- return true; >+ tree fntype = gimple_call_fntype (stmt); >+ tree dtype = TREE_TYPE (fndecl); >+ if (gimple_call_lhs (stmt) >+ && !useless_type_conversion_p (TREE_TYPE (fntype), >+ TREE_TYPE (dtype))) >+ final = false; >+ else >+ { >+ tree t1, t2; >+ for (t1 = TYPE_ARG_TYPES (fntype), >+ t2 = TYPE_ARG_TYPES (dtype); >+ t1 && t2; >+ t1 = TREE_CHAIN (t1), t2 = TREE_CHAIN (t2)) >+ if (!useless_type_conversion_p (TREE_VALUE (t2), >+ TREE_VALUE (t1))) >+ break; >+ if (t1 || t2) >+ final = false; >+ } >+ >+ /* If fndecl (like __builtin_unreachable or >+ __cxa_pure_virtual) takes no arguments, doesn't have >+ return value and is noreturn, just add the call before >+ stmt and DCE will do it's job later on. */ >+ if (!final >+ && !inplace >+ && (new_flags & ECF_NORETURN) >+ && VOID_TYPE_P (TREE_TYPE (dtype)) >+ && TYPE_ARG_TYPES (dtype) == void_list_node) >+ { >+ gimple new_stmt = gimple_build_call (fndecl, 0); >+ gimple_set_location (new_stmt, gimple_location (stmt)); >+ gsi_insert_before (gsi, new_stmt, GSI_SAME_STMT); >+ return true; >+ } >+ } >+ if (final) >+ { >+ gimple_call_set_fndecl (stmt, fndecl); >+ changed = true; > } > } > } >--- gcc/testsuite/g++.dg/opt/pr59622-2.C.jj 2014-01-02 >18:24:30.422832814 +0100 >+++ gcc/testsuite/g++.dg/opt/pr59622-2.C 2014-01-02 18:33:29.052037954 >+0100 >@@ -0,0 +1,21 @@ >+// PR tree-optimization/59622 >+// { dg-do compile } >+// { dg-options "-O2" } >+ >+namespace >+{ >+ struct A >+ { >+ A () {} >+ virtual A *bar (int) = 0; >+ A *baz (int x) { return bar (x); } >+ }; >+} >+ >+A *a; >+ >+void >+foo () >+{ >+ a->baz (0); >+} > > > Jakub
On Fri, Jan 03, 2014 at 10:26:44AM +0100, Richard Biener wrote: > Jakub Jelinek <jakub@redhat.com> wrote: > >On Tue, Dec 31, 2013 at 11:30:12AM +0100, Richard Biener wrote: > >> Meanwhile your patch is ok. > > > >As discussed in the PR, the patch wasn't sufficient, __cxa_pure_virtual > >can appear in the vtable and it has pretty much the same properties > >as __builtin_unreachable (void return value, no arguments, noreturn, > >DCE or cfg cleanup being able to remove anything after it because of > >noreturn). > > > >Additionally, the patch attempts to punt on invalid type changes > >(ODR violations?, apparently none appear during bootstrap/regtest as > >verified by additional logging added there) or inplace changes of > >gimple_call_flags that would require some cleanups caller isn't > >expected to > >do (again, doesn't happen during bootstrap/regtest). > > > >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > I'd rather do nothing in the !inplace case, it's rare enough to not care. Ok. > Also there is nothing wrong with wrong types et al - we have a perfect mechanism for dealing with this. Change the fndecl but not the fntype. Well, see PR59630. The question is if having to handle it everywhere is worth it. Jakub
On Fri, 3 Jan 2014, Jakub Jelinek wrote: > On Fri, Jan 03, 2014 at 10:26:44AM +0100, Richard Biener wrote: > > Jakub Jelinek <jakub@redhat.com> wrote: > > >On Tue, Dec 31, 2013 at 11:30:12AM +0100, Richard Biener wrote: > > >> Meanwhile your patch is ok. > > > > > >As discussed in the PR, the patch wasn't sufficient, __cxa_pure_virtual > > >can appear in the vtable and it has pretty much the same properties > > >as __builtin_unreachable (void return value, no arguments, noreturn, > > >DCE or cfg cleanup being able to remove anything after it because of > > >noreturn). > > > > > >Additionally, the patch attempts to punt on invalid type changes > > >(ODR violations?, apparently none appear during bootstrap/regtest as > > >verified by additional logging added there) or inplace changes of > > >gimple_call_flags that would require some cleanups caller isn't > > >expected to > > >do (again, doesn't happen during bootstrap/regtest). > > > > > >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > > > I'd rather do nothing in the !inplace case, it's rare enough to not care. > > Ok. > > > Also there is nothing wrong with wrong types et al - we have a perfect mechanism for dealing with this. Change the fndecl but not the fntype. > > Well, see PR59630. The question is if having to handle it everywhere > is worth it. Well, this case happens because we go back to GENERIC which doesn't have this feature. So "everywhere" is somewhat a broad stmt. It's easy to guard the builtins.c folding with a compatibility check of fntype and fndecl type. Richard.
On Fri, Jan 03, 2014 at 11:01:13AM +0100, Richard Biener wrote: > > Well, see PR59630. The question is if having to handle it everywhere > > is worth it. > > Well, this case happens because we go back to GENERIC which doesn't > have this feature. So "everywhere" is somewhat a broad stmt. > It's easy to guard the builtins.c folding with a compatibility > check of fntype and fndecl type. Well, clearly the inliner has similar issue, I doubt e.g. IPA cloning handles it right, there is just one gimple_call_fntype call in all of ipa*.c (which surprises me) and that ignores it if there is a decl: tree type = (e->callee ? TREE_TYPE (e->callee->decl) : gimple_call_fntype (e->call_stmt)); so if there is a mismatch between TREE_TYPE (e->callee->decl) and gimple_call_fntype, it will happily look at the decl type. So I'd say it is just a matter of adding more (invalid) testcases. Jakub
On Fri, 3 Jan 2014, Jakub Jelinek wrote: > On Fri, Jan 03, 2014 at 11:01:13AM +0100, Richard Biener wrote: > > > Well, see PR59630. The question is if having to handle it everywhere > > > is worth it. > > > > Well, this case happens because we go back to GENERIC which doesn't > > have this feature. So "everywhere" is somewhat a broad stmt. > > It's easy to guard the builtins.c folding with a compatibility > > check of fntype and fndecl type. > > Well, clearly the inliner has similar issue, I doubt e.g. IPA cloning > handles it right, there is just one gimple_call_fntype call in all of ipa*.c > (which surprises me) and that ignores it if there is a decl: > tree type = (e->callee > ? TREE_TYPE (e->callee->decl) > : gimple_call_fntype (e->call_stmt)); > so if there is a mismatch between TREE_TYPE (e->callee->decl) and > gimple_call_fntype, it will happily look at the decl type. > So I'd say it is just a matter of adding more (invalid) testcases. Of course we have to fix al the bugs. The inliner has a compatibility check in some of it's can-inline predicates. ipa_get_callee_param_type looks correct if it really wants to get at the param type of the _callee_ (not the type the caller uses!) static tree ipa_get_callee_param_type (struct cgraph_edge *e, int i) { int n; tree type = (e->callee ? TREE_TYPE (e->callee->decl) : gimple_call_fntype (e->call_stmt)); tree t = TYPE_ARG_TYPES (type); so if there is a decl then use its type signature, otherwise (indirect calls) use the caller signature (and hope it matches the callee...). That it later falls back to looking at DECL_ARGUMENTS is odd (probably a FE issue where we have a fndecl with a bogus type?) Richard.
On Fri, Jan 03, 2014 at 11:15:32AM +0100, Richard Biener wrote: > so if there is a decl then use its type signature, otherwise > (indirect calls) use the caller signature (and hope it matches > the callee...). That it later falls back to looking at > DECL_ARGUMENTS is odd (probably a FE issue where we have a > fndecl with a bogus type?) For K&R C non-prototyped functions foo (x, y) int x, y; { return x + y; } I think TYPE_ARG_TYPES is NULL (?) and DECL_ARGUMENTS is the only way to get at the types. Jakub
On Fri, 3 Jan 2014, Jakub Jelinek wrote: > On Fri, Jan 03, 2014 at 11:15:32AM +0100, Richard Biener wrote: > > so if there is a decl then use its type signature, otherwise > > (indirect calls) use the caller signature (and hope it matches > > the callee...). That it later falls back to looking at > > DECL_ARGUMENTS is odd (probably a FE issue where we have a > > fndecl with a bogus type?) > > For K&R C non-prototyped functions > foo (x, y) > int x, y; > { > return x + y; > } > I think TYPE_ARG_TYPES is NULL (?) and DECL_ARGUMENTS is the only way to get > at the types. Ah, indeed. A C speciality that shouldn't survive GENERICization if that's really the case. I wonder how int main() { return (*(&foo))(0, 0); } works though. Or __auto_type x = foo; (substitute for that new C __auto thing). The former probably because we fold away the indirection very early. Richard.
On Fri, 3 Jan 2014, Richard Biener wrote: > On Fri, 3 Jan 2014, Jakub Jelinek wrote: > > > On Fri, Jan 03, 2014 at 11:15:32AM +0100, Richard Biener wrote: > > > so if there is a decl then use its type signature, otherwise > > > (indirect calls) use the caller signature (and hope it matches > > > the callee...). That it later falls back to looking at > > > DECL_ARGUMENTS is odd (probably a FE issue where we have a > > > fndecl with a bogus type?) > > > > For K&R C non-prototyped functions > > foo (x, y) > > int x, y; > > { > > return x + y; > > } > > I think TYPE_ARG_TYPES is NULL (?) and DECL_ARGUMENTS is the only way to get > > at the types. > > Ah, indeed. A C speciality that shouldn't survive GENERICization if > that's really the case. > > I wonder how > > int main() > { > return (*(&foo))(0, 0); > } > > works though. Or > > __auto_type x = foo; > > (substitute for that new C __auto thing). > > The former probably because we fold away the indirection very early. foo (x, y) int x, y; { return x + y; } int main() { __auto_type f = foo; return f(0,0); } works just fine for me but shows int (*<T391>) () f; int _4; <bb 2>: f_1 = foo; _4 = f_1 (0, 0); so I suppose the fntype is not NULL but int (*)(). Richard.
On Fri, Jan 03, 2014 at 11:24:53AM +0100, Richard Biener wrote: > works just fine for me but shows > > int (*<T391>) () f; > int _4; > > <bb 2>: > f_1 = foo; > _4 = f_1 (0, 0); > > so I suppose the fntype is not NULL but int (*)(). Sure, I said TYPE_ARG_TYPES is NULL, which is the () there. Jakub
--- gcc/gimple-fold.c.jj 2013-12-31 12:56:59.000000000 +0100 +++ gcc/gimple-fold.c 2014-01-02 18:33:51.207921734 +0100 @@ -51,6 +51,7 @@ along with GCC; see the file COPYING3. #include "gimple-pretty-print.h" #include "tree-ssa-address.h" #include "langhooks.h" +#include "calls.h" /* Return true when DECL can be referenced from current unit. FROM_DECL (if non-null) specify constructor of variable DECL was taken from. @@ -1167,7 +1168,7 @@ gimple_fold_call (gimple_stmt_iterator * (OBJ_TYPE_REF_EXPR (callee))))) { fprintf (dump_file, - "Type inheritnace inconsistent devirtualization of "); + "Type inheritance inconsistent devirtualization of "); print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM); fprintf (dump_file, " to "); print_generic_expr (dump_file, callee, TDF_SLIM); @@ -1184,18 +1185,74 @@ gimple_fold_call (gimple_stmt_iterator * = possible_polymorphic_call_targets (callee, &final); if (final && targets.length () <= 1) { + tree fndecl; + int call_flags = gimple_call_flags (stmt), new_flags; if (targets.length () == 1) + fndecl = targets[0]->decl; + else + fndecl = builtin_decl_implicit (BUILT_IN_UNREACHABLE); + new_flags = flags_from_decl_or_type (fndecl); + if (inplace) { - gimple_call_set_fndecl (stmt, targets[0]->decl); - changed = true; + /* For inplace, avoid changes of call flags that + might require cfg cleanups, EH cleanups, removal + of vdef/vuses etc. */ + if ((call_flags ^ new_flags) + & (ECF_NORETURN | ECF_NOTHROW | ECF_NOVOPS)) + final = false; + else if ((call_flags & ECF_NOVOPS) == 0) + { + if ((call_flags ^ new_flags) & ECF_CONST) + final = false; + else if (((call_flags & (ECF_PURE | ECF_CONST + | ECF_NORETURN)) == 0) + ^ ((new_flags & (ECF_PURE | ECF_CONST + | ECF_NORETURN)) == 0)) + final = false; + } } - else if (!inplace) + if (final) { - tree fndecl = builtin_decl_implicit (BUILT_IN_UNREACHABLE); - gimple new_stmt = gimple_build_call (fndecl, 0); - gimple_set_location (new_stmt, gimple_location (stmt)); - gsi_insert_before (gsi, new_stmt, GSI_SAME_STMT); - return true; + tree fntype = gimple_call_fntype (stmt); + tree dtype = TREE_TYPE (fndecl); + if (gimple_call_lhs (stmt) + && !useless_type_conversion_p (TREE_TYPE (fntype), + TREE_TYPE (dtype))) + final = false; + else + { + tree t1, t2; + for (t1 = TYPE_ARG_TYPES (fntype), + t2 = TYPE_ARG_TYPES (dtype); + t1 && t2; + t1 = TREE_CHAIN (t1), t2 = TREE_CHAIN (t2)) + if (!useless_type_conversion_p (TREE_VALUE (t2), + TREE_VALUE (t1))) + break; + if (t1 || t2) + final = false; + } + + /* If fndecl (like __builtin_unreachable or + __cxa_pure_virtual) takes no arguments, doesn't have + return value and is noreturn, just add the call before + stmt and DCE will do it's job later on. */ + if (!final + && !inplace + && (new_flags & ECF_NORETURN) + && VOID_TYPE_P (TREE_TYPE (dtype)) + && TYPE_ARG_TYPES (dtype) == void_list_node) + { + gimple new_stmt = gimple_build_call (fndecl, 0); + gimple_set_location (new_stmt, gimple_location (stmt)); + gsi_insert_before (gsi, new_stmt, GSI_SAME_STMT); + return true; + } + } + if (final) + { + gimple_call_set_fndecl (stmt, fndecl); + changed = true; } } } --- gcc/testsuite/g++.dg/opt/pr59622-2.C.jj 2014-01-02 18:24:30.422832814 +0100 +++ gcc/testsuite/g++.dg/opt/pr59622-2.C 2014-01-02 18:33:29.052037954 +0100 @@ -0,0 +1,21 @@ +// PR tree-optimization/59622 +// { dg-do compile } +// { dg-options "-O2" } + +namespace +{ + struct A + { + A () {} + virtual A *bar (int) = 0; + A *baz (int x) { return bar (x); } + }; +} + +A *a; + +void +foo () +{ + a->baz (0); +}
On Tue, Dec 31, 2013 at 11:30:12AM +0100, Richard Biener wrote: > Meanwhile your patch is ok. As discussed in the PR, the patch wasn't sufficient, __cxa_pure_virtual can appear in the vtable and it has pretty much the same properties as __builtin_unreachable (void return value, no arguments, noreturn, DCE or cfg cleanup being able to remove anything after it because of noreturn). Additionally, the patch attempts to punt on invalid type changes (ODR violations?, apparently none appear during bootstrap/regtest as verified by additional logging added there) or inplace changes of gimple_call_flags that would require some cleanups caller isn't expected to do (again, doesn't happen during bootstrap/regtest). Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2014-01-03 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/59622 * gimple-fold.c: Include calls.h. (gimple_fold_call): Fix a typo in message. Handle __cxa_pure_virtual similarly to __builtin_unreachable. For inplace punt if gimple_call_flags would change. Verify that lhs and argument types are compatible. * g++.dg/opt/pr59622-2.C: New test. Jakub