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)
