Message ID | 20211113111552.GB15769@kam.mff.cuni.cz |
---|---|
State | New |
Headers | show |
Series | Enable ipa-sra for functions with fnspec attribute | expand |
Hi, On Sat, Nov 13 2021, Jan Hubicka via Gcc-patches wrote: > Hi, > this patch enables some ipa-sra on fortran by allowing signature changes on functions > with "fn spec" attribute when ipa-modref is enabled. This is possible since ipa-modref > knows how to preserve things we trace in fnspec and fnspec generated by fortran forntend > are quite simple and can be analysed automatically now. To be sure I will also add > code that merge fnspec to parameters. > > This unfortunately hits bug in ipa-param-manipulation when we remove parameter > that specifies size of variable length parameter. For this reason I added a hack > that prevent signature changes on such functions and will handle it incrementally. > > I tried creating C testcase but it is blocked by another problem that we punt ipa-sra > on access attribute. This is optimization regression we ought to fix so I filled > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103223. > > As a followup I will add code classifying the type attributes (we have just few) and > get stats on access attribute. > > Martin, can you please check that the code detecting signature changes is correct I think both ways of detecting it in the patch are correct, in the sense that if the clone in question does not modify parameters but is a clone of another clone which does, the methods would still consider it a changing clone (to do otherwise they would need to check prev_clone_index and not base_index). But I don't think the difference matters here. It might if we start updating the strings of the attributes where position is important. > ...and can't be done more easily? in the case of ipa_param_body_adjustments you could introduce another flag of the class and set it whenever any of the elements in local vector kept in common_initialization() turned out not to be true. Martin > > Bootstrapped/regtested x86_64-linux, comitted. > Honza > > gcc/ChangeLog: > > * ipa-fnsummary.c (compute_fn_summary): Do not give up on signature > changes on "fn spec" attribute; give up on varadic types. > * ipa-param-manipulation.c: Include attribs.h. > (build_adjusted_function_type): New parameter ARG_MODIFIED; if it is > true remove "fn spec" attribute. > (ipa_param_adjustments::build_new_function_type): Update. > (ipa_param_body_adjustments::modify_formal_parameters): update. > * ipa-sra.c: Include attribs.h. > (ipa_sra_preliminary_function_checks): Do not check for TYPE_ATTRIBUTES. > > diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c > index 2cfa9a6d0e9..94a80d3ec90 100644 > --- a/gcc/ipa-fnsummary.c > +++ b/gcc/ipa-fnsummary.c > @@ -3135,10 +3135,38 @@ compute_fn_summary (struct cgraph_node *node, bool early) > else > info->inlinable = tree_inlinable_function_p (node->decl); > > - /* Type attributes can use parameter indices to describe them. */ > - if (TYPE_ATTRIBUTES (TREE_TYPE (node->decl)) > - /* Likewise for #pragma omp declare simd functions or functions > - with simd attribute. */ > + bool no_signature = false; > + /* Type attributes can use parameter indices to describe them. > + Special case fn spec since we can safely preserve them in > + modref summaries. */ > + for (tree list = TYPE_ATTRIBUTES (TREE_TYPE (node->decl)); > + list && !no_signature; list = TREE_CHAIN (list)) > + if (!flag_ipa_modref > + || !is_attribute_p ("fn spec", get_attribute_name (list))) > + { > + if (dump_file) > + { > + fprintf (dump_file, "No signature change:" > + " function type has unhandled attribute %s.\n", > + IDENTIFIER_POINTER (get_attribute_name (list))); > + } > + no_signature = true; > + } > + for (tree parm = DECL_ARGUMENTS (node->decl); > + parm && !no_signature; parm = DECL_CHAIN (parm)) > + if (variably_modified_type_p (TREE_TYPE (parm), node->decl)) > + { > + if (dump_file) > + { > + fprintf (dump_file, "No signature change:" > + " has parameter with variably modified type.\n"); > + } > + no_signature = true; > + } > + > + /* Likewise for #pragma omp declare simd functions or functions > + with simd attribute. */ > + if (no_signature > || lookup_attribute ("omp declare simd", > DECL_ATTRIBUTES (node->decl))) > node->can_change_signature = false; > diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c > index ae3149718ca..20f41dd5363 100644 > --- a/gcc/ipa-param-manipulation.c > +++ b/gcc/ipa-param-manipulation.c > @@ -45,6 +45,7 @@ along with GCC; see the file COPYING3. If not see > #include "symtab-clones.h" > #include "tree-phinodes.h" > #include "cfgexpand.h" > +#include "attribs.h" > > > /* Actual prefixes of different newly synthetized parameters. Keep in sync > @@ -281,11 +282,13 @@ fill_vector_of_new_param_types (vec<tree> *new_types, vec<tree> *otypes, > /* Build and return a function type just like ORIG_TYPE but with parameter > types given in NEW_PARAM_TYPES - which can be NULL if, but only if, > ORIG_TYPE itself has NULL TREE_ARG_TYPEs. If METHOD2FUNC is true, also make > - it a FUNCTION_TYPE instead of FUNCTION_TYPE. */ > + it a FUNCTION_TYPE instead of FUNCTION_TYPE. > + If ARG_MODIFIED is true drop attributes that are no longer up to date. */ > > static tree > build_adjusted_function_type (tree orig_type, vec<tree> *new_param_types, > - bool method2func, bool skip_return) > + bool method2func, bool skip_return, > + bool args_modified) > { > tree new_arg_types = NULL; > if (TYPE_ARG_TYPES (orig_type)) > @@ -334,6 +337,17 @@ build_adjusted_function_type (tree orig_type, vec<tree> *new_param_types, > if (skip_return) > TREE_TYPE (new_type) = void_type_node; > } > + /* We only support one fn spec attribute on type. Be sure to remove it. > + Once we support multiple attributes we will need to be able to unshare > + the list. */ > + if (args_modified && TYPE_ATTRIBUTES (new_type)) > + { > + gcc_checking_assert > + (!TREE_CHAIN (TYPE_ATTRIBUTES (new_type)) > + && (is_attribute_p ("fn spec", > + get_attribute_name (TYPE_ATTRIBUTES (new_type))))); > + TYPE_ATTRIBUTES (new_type) = NULL; > + } > > return new_type; > } > @@ -460,8 +474,22 @@ ipa_param_adjustments::build_new_function_type (tree old_type, > else > new_param_types_p = NULL; > > + /* Check if any params type cares about are modified. In this case will > + need to drop some type attributes. */ > + bool modified = false; > + size_t index = 0; > + if (m_adj_params) > + for (tree t = TYPE_ARG_TYPES (old_type); > + t && (int)index < m_always_copy_start && !modified; > + t = TREE_CHAIN (t), index++) > + if (index >= m_adj_params->length () > + || get_original_index (index) != (int)index) > + modified = true; > + > + > return build_adjusted_function_type (old_type, new_param_types_p, > - method2func_p (old_type), m_skip_return); > + method2func_p (old_type), m_skip_return, > + modified); > } > > /* Build variant of function decl ORIG_DECL which has no return value if > @@ -1467,12 +1495,23 @@ ipa_param_body_adjustments::modify_formal_parameters () > if (fndecl_built_in_p (m_fndecl)) > set_decl_built_in_function (m_fndecl, NOT_BUILT_IN, 0); > > + bool modified = false; > + size_t index = 0; > + if (m_adj_params) > + for (tree t = TYPE_ARG_TYPES (orig_type); > + t && !modified; > + t = TREE_CHAIN (t), index++) > + if (index >= m_adj_params->length () > + || (*m_adj_params)[index].op != IPA_PARAM_OP_COPY > + || (*m_adj_params)[index].base_index != index) > + modified = true; > + > /* At this point, removing return value is only implemented when going > through tree_function_versioning, not when modifying function body > directly. */ > gcc_assert (!m_adjustments || !m_adjustments->m_skip_return); > tree new_type = build_adjusted_function_type (orig_type, &m_new_types, > - m_method2func, false); > + m_method2func, false, modified); > > TREE_TYPE (m_fndecl) = new_type; > DECL_VIRTUAL_P (m_fndecl) = 0; > diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c > index 88036590425..cb0e30507a1 100644 > --- a/gcc/ipa-sra.c > +++ b/gcc/ipa-sra.c > @@ -85,6 +85,7 @@ along with GCC; see the file COPYING3. If not see > #include "tree-streamer.h" > #include "internal-fn.h" > #include "symtab-clones.h" > +#include "attribs.h" > > static void ipa_sra_summarize_function (cgraph_node *); > > @@ -616,13 +617,6 @@ ipa_sra_preliminary_function_checks (cgraph_node *node) > return false; > } > > - if (TYPE_ATTRIBUTES (TREE_TYPE (node->decl))) > - { > - if (dump_file) > - fprintf (dump_file, "Function type has attributes. \n"); > - return false; > - } > - > if (DECL_DISREGARD_INLINE_LIMITS (node->decl)) > { > if (dump_file)
diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c index 2cfa9a6d0e9..94a80d3ec90 100644 --- a/gcc/ipa-fnsummary.c +++ b/gcc/ipa-fnsummary.c @@ -3135,10 +3135,38 @@ compute_fn_summary (struct cgraph_node *node, bool early) else info->inlinable = tree_inlinable_function_p (node->decl); - /* Type attributes can use parameter indices to describe them. */ - if (TYPE_ATTRIBUTES (TREE_TYPE (node->decl)) - /* Likewise for #pragma omp declare simd functions or functions - with simd attribute. */ + bool no_signature = false; + /* Type attributes can use parameter indices to describe them. + Special case fn spec since we can safely preserve them in + modref summaries. */ + for (tree list = TYPE_ATTRIBUTES (TREE_TYPE (node->decl)); + list && !no_signature; list = TREE_CHAIN (list)) + if (!flag_ipa_modref + || !is_attribute_p ("fn spec", get_attribute_name (list))) + { + if (dump_file) + { + fprintf (dump_file, "No signature change:" + " function type has unhandled attribute %s.\n", + IDENTIFIER_POINTER (get_attribute_name (list))); + } + no_signature = true; + } + for (tree parm = DECL_ARGUMENTS (node->decl); + parm && !no_signature; parm = DECL_CHAIN (parm)) + if (variably_modified_type_p (TREE_TYPE (parm), node->decl)) + { + if (dump_file) + { + fprintf (dump_file, "No signature change:" + " has parameter with variably modified type.\n"); + } + no_signature = true; + } + + /* Likewise for #pragma omp declare simd functions or functions + with simd attribute. */ + if (no_signature || lookup_attribute ("omp declare simd", DECL_ATTRIBUTES (node->decl))) node->can_change_signature = false; diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c index ae3149718ca..20f41dd5363 100644 --- a/gcc/ipa-param-manipulation.c +++ b/gcc/ipa-param-manipulation.c @@ -45,6 +45,7 @@ along with GCC; see the file COPYING3. If not see #include "symtab-clones.h" #include "tree-phinodes.h" #include "cfgexpand.h" +#include "attribs.h" /* Actual prefixes of different newly synthetized parameters. Keep in sync @@ -281,11 +282,13 @@ fill_vector_of_new_param_types (vec<tree> *new_types, vec<tree> *otypes, /* Build and return a function type just like ORIG_TYPE but with parameter types given in NEW_PARAM_TYPES - which can be NULL if, but only if, ORIG_TYPE itself has NULL TREE_ARG_TYPEs. If METHOD2FUNC is true, also make - it a FUNCTION_TYPE instead of FUNCTION_TYPE. */ + it a FUNCTION_TYPE instead of FUNCTION_TYPE. + If ARG_MODIFIED is true drop attributes that are no longer up to date. */ static tree build_adjusted_function_type (tree orig_type, vec<tree> *new_param_types, - bool method2func, bool skip_return) + bool method2func, bool skip_return, + bool args_modified) { tree new_arg_types = NULL; if (TYPE_ARG_TYPES (orig_type)) @@ -334,6 +337,17 @@ build_adjusted_function_type (tree orig_type, vec<tree> *new_param_types, if (skip_return) TREE_TYPE (new_type) = void_type_node; } + /* We only support one fn spec attribute on type. Be sure to remove it. + Once we support multiple attributes we will need to be able to unshare + the list. */ + if (args_modified && TYPE_ATTRIBUTES (new_type)) + { + gcc_checking_assert + (!TREE_CHAIN (TYPE_ATTRIBUTES (new_type)) + && (is_attribute_p ("fn spec", + get_attribute_name (TYPE_ATTRIBUTES (new_type))))); + TYPE_ATTRIBUTES (new_type) = NULL; + } return new_type; } @@ -460,8 +474,22 @@ ipa_param_adjustments::build_new_function_type (tree old_type, else new_param_types_p = NULL; + /* Check if any params type cares about are modified. In this case will + need to drop some type attributes. */ + bool modified = false; + size_t index = 0; + if (m_adj_params) + for (tree t = TYPE_ARG_TYPES (old_type); + t && (int)index < m_always_copy_start && !modified; + t = TREE_CHAIN (t), index++) + if (index >= m_adj_params->length () + || get_original_index (index) != (int)index) + modified = true; + + return build_adjusted_function_type (old_type, new_param_types_p, - method2func_p (old_type), m_skip_return); + method2func_p (old_type), m_skip_return, + modified); } /* Build variant of function decl ORIG_DECL which has no return value if @@ -1467,12 +1495,23 @@ ipa_param_body_adjustments::modify_formal_parameters () if (fndecl_built_in_p (m_fndecl)) set_decl_built_in_function (m_fndecl, NOT_BUILT_IN, 0); + bool modified = false; + size_t index = 0; + if (m_adj_params) + for (tree t = TYPE_ARG_TYPES (orig_type); + t && !modified; + t = TREE_CHAIN (t), index++) + if (index >= m_adj_params->length () + || (*m_adj_params)[index].op != IPA_PARAM_OP_COPY + || (*m_adj_params)[index].base_index != index) + modified = true; + /* At this point, removing return value is only implemented when going through tree_function_versioning, not when modifying function body directly. */ gcc_assert (!m_adjustments || !m_adjustments->m_skip_return); tree new_type = build_adjusted_function_type (orig_type, &m_new_types, - m_method2func, false); + m_method2func, false, modified); TREE_TYPE (m_fndecl) = new_type; DECL_VIRTUAL_P (m_fndecl) = 0; diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c index 88036590425..cb0e30507a1 100644 --- a/gcc/ipa-sra.c +++ b/gcc/ipa-sra.c @@ -85,6 +85,7 @@ along with GCC; see the file COPYING3. If not see #include "tree-streamer.h" #include "internal-fn.h" #include "symtab-clones.h" +#include "attribs.h" static void ipa_sra_summarize_function (cgraph_node *); @@ -616,13 +617,6 @@ ipa_sra_preliminary_function_checks (cgraph_node *node) return false; } - if (TYPE_ATTRIBUTES (TREE_TYPE (node->decl))) - { - if (dump_file) - fprintf (dump_file, "Function type has attributes. \n"); - return false; - } - if (DECL_DISREGARD_INLINE_LIMITS (node->decl)) { if (dump_file)