Re: RFA (make_dispatcher_decl): PATCH for c++/83911, ICE with multiversioned constructor
On Fri, Mar 16, 2018 at 8:38 AM, Jason Merrillwrote: > On Thu, Mar 15, 2018 at 4:50 AM, Richard Biener > wrote: >> On Wed, Mar 14, 2018 at 8:57 PM, Jason Merrill wrote: >>> Ping >>> >>> On Fri, Mar 2, 2018 at 1:23 PM, Jason Merrill wrote: As I mentioned in the PR, the problem here is that we're replacing a constructor with a dispatcher function which doesn't look much like a constructor. This patch adjusts make_dispatcher_decl to make it look more like the functions it dispatches to, but other things are certain to break for similar reasons down the road. A proper solution should be more transparent, like thunks. Tested x86_64-pc-linux-gnu. Does this seem worth applying to fix the regression? >> >> The patch looks reasonable to me, you probably know best whether >> the cp/ parts are risky or not ;) >> >> So - OK from my POV. >> >> And yes, thunks may be a better representation for the dispatcher. > > It occurred to me that I could handle this more locally by deferring > the function substitution until genericization time, so this is what > I'm checking in: ...but now that we're in stage 1, it still seems sensible to have a single way of checking whether something is a constructor. Tested x86_64-pc-linux-gnu, applying to trunk. commit 656d038fe0cf78a2432a8c9a047edc93af6d5b23 Author: Jason Merrill Date: Fri Mar 16 08:32:26 2018 -0400 * cp-tree.h (DECL_CONSTRUCTOR_P): Use DECL_CXX_CONSTRUCTOR_P. (DECL_DESTRUCTOR_P): Use DECL_CXX_DESTRUCTOR_P. diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 2df158c9ea6..a4e0099a249 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -2731,7 +2731,7 @@ struct GTY(()) lang_decl { /* For FUNCTION_DECLs and TEMPLATE_DECLs: nonzero means that this function is a constructor. */ #define DECL_CONSTRUCTOR_P(NODE) \ - IDENTIFIER_CTOR_P (DECL_NAME (NODE)) + DECL_CXX_CONSTRUCTOR_P (STRIP_TEMPLATE (NODE)) /* Nonzero if NODE (a FUNCTION_DECL) is a constructor for a complete object. */ @@ -2760,7 +2760,7 @@ struct GTY(()) lang_decl { /* Nonzero if NODE (a FUNCTION_DECL or TEMPLATE_DECL) is a destructor. */ #define DECL_DESTRUCTOR_P(NODE)\ - IDENTIFIER_DTOR_P (DECL_NAME (NODE)) + DECL_CXX_DESTRUCTOR_P (STRIP_TEMPLATE (NODE)) /* Nonzero if NODE (a FUNCTION_DECL) is a destructor, but not the specialized in-charge constructor, in-charge deleting constructor,
Re: RFA (make_dispatcher_decl): PATCH for c++/83911, ICE with multiversioned constructor
On Thu, Mar 15, 2018 at 4:50 AM, Richard Bienerwrote: > On Wed, Mar 14, 2018 at 8:57 PM, Jason Merrill wrote: >> Ping >> >> On Fri, Mar 2, 2018 at 1:23 PM, Jason Merrill wrote: >>> As I mentioned in the PR, the problem here is that we're replacing a >>> constructor with a dispatcher function which doesn't look much like a >>> constructor. This patch adjusts make_dispatcher_decl to make it look >>> more like the functions it dispatches to, but other things are certain >>> to break for similar reasons down the road. A proper solution should >>> be more transparent, like thunks. >>> >>> Tested x86_64-pc-linux-gnu. Does this seem worth applying to fix the >>> regression? > > The patch looks reasonable to me, you probably know best whether > the cp/ parts are risky or not ;) > > So - OK from my POV. > > And yes, thunks may be a better representation for the dispatcher. It occurred to me that I could handle this more locally by deferring the function substitution until genericization time, so this is what I'm checking in: commit 05dda93e56e02859c65f93bd541dc7793bc7305c Author: Jason Merrill Date: Fri Feb 16 16:53:47 2018 -0500 PR c++/83911 - ICE with multiversioned constructor. * cp-gimplify.c (cp_genericize_r): Replace versioned function with dispatchere here. * call.c (build_over_call): Not here. PR c++/83911 - ICE with multiversioned constructor. diff --git a/gcc/cp/call.c b/gcc/cp/call.c index 45c22aaa312..67438ff2e94 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -8204,23 +8204,6 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain) } } - /* For calls to a multi-versioned function, overload resolution - returns the function with the highest target priority, that is, - the version that will checked for dispatching first. If this - version is inlinable, a direct call to this version can be made - otherwise the call should go through the dispatcher. */ - - if (DECL_FUNCTION_VERSIONED (fn) - && (current_function_decl == NULL - || !targetm.target_option.can_inline_p (current_function_decl, fn))) -{ - fn = get_function_version_dispatcher (fn); - if (fn == NULL) - return NULL; - if (!already_used) - mark_versions_used (fn); -} - if (!already_used && !mark_used (fn, complain)) return error_mark_node; diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c index 0ddd435454c..653d1dcee26 100644 --- a/gcc/cp/cp-gimplify.c +++ b/gcc/cp/cp-gimplify.c @@ -1513,6 +1513,29 @@ cp_genericize_r (tree *stmt_p, int *walk_subtrees, void *data) == REFERENCE_TYPE)) *walk_subtrees = 0; } + /* Fall through. */ +case AGGR_INIT_EXPR: + /* For calls to a multi-versioned function, overload resolution + returns the function with the highest target priority, that is, + the version that will checked for dispatching first. If this + version is inlinable, a direct call to this version can be made + otherwise the call should go through the dispatcher. */ + { + tree fn = cp_get_callee_fndecl (stmt); + if (fn && DECL_FUNCTION_VERSIONED (fn) + && (current_function_decl == NULL + || !targetm.target_option.can_inline_p (current_function_decl, + fn))) + if (tree dis = get_function_version_dispatcher (fn)) + { + mark_versions_used (dis); + dis = build_address (dis); + if (TREE_CODE (stmt) == CALL_EXPR) + CALL_EXPR_FN (stmt) = dis; + else + AGGR_INIT_EXPR_FN (stmt) = dis; + } + } break; default: diff --git a/gcc/testsuite/g++.dg/ext/mv27.C b/gcc/testsuite/g++.dg/ext/mv27.C new file mode 100644 index 000..443a54be765 --- /dev/null +++ b/gcc/testsuite/g++.dg/ext/mv27.C @@ -0,0 +1,18 @@ +// PR c++/83911 +// { dg-do compile { target i?86-*-* x86_64-*-* } } +// { dg-require-ifunc "" } + +class SimdFloat +{ +public: +__attribute__ ((target ("default"))) +SimdFloat(float x) {} + +__attribute__ ((target ("avx2"))) +SimdFloat(float x) {} +}; + +SimdFloat foo() +{ +return 1; +}
Re: RFA (make_dispatcher_decl): PATCH for c++/83911, ICE with multiversioned constructor
On Wed, Mar 14, 2018 at 8:57 PM, Jason Merrillwrote: > Ping > > On Fri, Mar 2, 2018 at 1:23 PM, Jason Merrill wrote: >> As I mentioned in the PR, the problem here is that we're replacing a >> constructor with a dispatcher function which doesn't look much like a >> constructor. This patch adjusts make_dispatcher_decl to make it look >> more like the functions it dispatches to, but other things are certain >> to break for similar reasons down the road. A proper solution should >> be more transparent, like thunks. >> >> Tested x86_64-pc-linux-gnu. Does this seem worth applying to fix the >> regression? The patch looks reasonable to me, you probably know best whether the cp/ parts are risky or not ;) So - OK from my POV. And yes, thunks may be a better representation for the dispatcher. Richard.
Re: RFA (make_dispatcher_decl): PATCH for c++/83911, ICE with multiversioned constructor
Ping On Fri, Mar 2, 2018 at 1:23 PM, Jason Merrillwrote: > As I mentioned in the PR, the problem here is that we're replacing a > constructor with a dispatcher function which doesn't look much like a > constructor. This patch adjusts make_dispatcher_decl to make it look > more like the functions it dispatches to, but other things are certain > to break for similar reasons down the road. A proper solution should > be more transparent, like thunks. > > Tested x86_64-pc-linux-gnu. Does this seem worth applying to fix the > regression?
RFA (make_dispatcher_decl): PATCH for c++/83911, ICE with multiversioned constructor
As I mentioned in the PR, the problem here is that we're replacing a constructor with a dispatcher function which doesn't look much like a constructor. This patch adjusts make_dispatcher_decl to make it look more like the functions it dispatches to, but other things are certain to break for similar reasons down the road. A proper solution should be more transparent, like thunks. Tested x86_64-pc-linux-gnu. Does this seem worth applying to fix the regression? commit d3b4453834bb2e3cc1c875e76fe593c16f263f77 Author: Jason MerrillDate: Fri Feb 16 16:53:47 2018 -0500 PR c++/83911 - ICE with multiversioned constructor. gcc/ * attribs.c (make_dispatcher_decl): Copy METHOD_TYPE, DECL_CXX_{CON,DE}STRUCTOR_P. gcc/cp/ * cp-tree.h (DECL_CONSTRUCTOR_P): Use DECL_CXX_CONSTRUCTOR_P. (DECL_DESTRUCTOR_P): Use DECL_CXX_DESTRUCTOR_P. * mangle.c (write_unqualified_name): Check IDENTIFIER_[CD]TOR_P. diff --git a/gcc/attribs.c b/gcc/attribs.c index bfadf124dcb..2e9e69904f0 100644 --- a/gcc/attribs.c +++ b/gcc/attribs.c @@ -1063,9 +1063,14 @@ make_dispatcher_decl (const tree decl) func_name = xstrdup (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl))); fn_type = TREE_TYPE (decl); - func_type = build_function_type (TREE_TYPE (fn_type), - TYPE_ARG_TYPES (fn_type)); - + if (TREE_CODE (fn_type) == METHOD_TYPE) +func_type = build_method_type_directly (TYPE_METHOD_BASETYPE (fn_type), + TREE_TYPE (fn_type), + TYPE_ARG_TYPES (fn_type)); + else +func_type = build_function_type (TREE_TYPE (fn_type), +TYPE_ARG_TYPES (fn_type)); + func_decl = build_fn_decl (func_name, func_type); XDELETEVEC (func_name); TREE_USED (func_decl) = 1; @@ -1078,6 +1083,9 @@ make_dispatcher_decl (const tree decl) /* This will be of type IFUNCs have to be externally visible. */ TREE_PUBLIC (func_decl) = 1; + DECL_CXX_CONSTRUCTOR_P (func_decl) = DECL_CXX_CONSTRUCTOR_P (decl); + DECL_CXX_DESTRUCTOR_P (func_decl) = DECL_CXX_DESTRUCTOR_P (decl); + return func_decl; } diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 17d8c6d2650..268be0fd543 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -2695,7 +2695,7 @@ struct GTY(()) lang_decl { /* For FUNCTION_DECLs and TEMPLATE_DECLs: nonzero means that this function is a constructor. */ #define DECL_CONSTRUCTOR_P(NODE) \ - IDENTIFIER_CTOR_P (DECL_NAME (NODE)) + DECL_CXX_CONSTRUCTOR_P (STRIP_TEMPLATE (NODE)) /* Nonzero if NODE (a FUNCTION_DECL) is a constructor for a complete object. */ @@ -2724,7 +2724,7 @@ struct GTY(()) lang_decl { /* Nonzero if NODE (a FUNCTION_DECL or TEMPLATE_DECL) is a destructor. */ #define DECL_DESTRUCTOR_P(NODE)\ - IDENTIFIER_DTOR_P (DECL_NAME (NODE)) + DECL_CXX_DESTRUCTOR_P (STRIP_TEMPLATE (NODE)) /* Nonzero if NODE (a FUNCTION_DECL) is a destructor, but not the specialized in-charge constructor, in-charge deleting constructor, diff --git a/gcc/cp/mangle.c b/gcc/cp/mangle.c index 94c4bed2848..ecd4eb066d4 100644 --- a/gcc/cp/mangle.c +++ b/gcc/cp/mangle.c @@ -1351,9 +1351,9 @@ write_unqualified_name (tree decl) else if (DECL_DECLARES_FUNCTION_P (decl)) { found = true; - if (DECL_CONSTRUCTOR_P (decl)) + if (IDENTIFIER_CTOR_P (DECL_NAME (decl))) write_special_name_constructor (decl); - else if (DECL_DESTRUCTOR_P (decl)) + else if (IDENTIFIER_DTOR_P (DECL_NAME (decl))) write_special_name_destructor (decl); else if (DECL_CONV_FN_P (decl)) { diff --git a/gcc/testsuite/g++.dg/ext/mv27.C b/gcc/testsuite/g++.dg/ext/mv27.C new file mode 100644 index 000..443a54be765 --- /dev/null +++ b/gcc/testsuite/g++.dg/ext/mv27.C @@ -0,0 +1,18 @@ +// PR c++/83911 +// { dg-do compile { target i?86-*-* x86_64-*-* } } +// { dg-require-ifunc "" } + +class SimdFloat +{ +public: +__attribute__ ((target ("default"))) +SimdFloat(float x) {} + +__attribute__ ((target ("avx2"))) +SimdFloat(float x) {} +}; + +SimdFloat foo() +{ +return 1; +}