On Thu, Feb 2, 2023 at 5:20 PM Martin Jambor <mjam...@suse.cz> wrote:
>
> Hi,
>
> when the compiled program contains type mismatches between callers and
> callees when it comes to a parameter, IPA-CP can try to propagate one
> constant from callers while IPA-SRA may try to split a parameter
> expecting a value of a different size on the same offset. This then
> currently leads to creation of a VIEW_CONVERT_EXPR with mismatching
> type sizes of LHS and RHS which is correctly flagged by the GIMPLE
> verifier as invalid.
>
> It seems that the best course of action is to try and avoid the
> situation altogether and so this patch adds a check to IPA-SRA that
> peeks into the result of IPA-CP and when it sees a value on the same
> offset but with a mismatching size, it just decides to leave that
> particular parameter be.
>
> Bootstrapped and tested on x86_64-linux, OK for master?
OK. I suppose there are guards elsewhere that never lets a
non-UHWI size type (like variable size or poly-int-size) through
any of the SRA or CP lattices?
Thanks,
Richard.
> Thanks,
>
> Martin
>
>
> gcc/ChangeLog:
>
> 2023-02-02 Martin Jambor <mjam...@suse.cz>
>
> PR ipa/108384
> * ipa-sra.cc (push_param_adjustments_for_index): Remove a size check
> when comparing to an IPA-CP value.
> (dump_list_of_param_indices): New function.
> (adjust_parameter_descriptions): Check for mismatching IPA-CP values.
> Dump removed candidates using dump_list_of_param_indices.
> * ipa-param-manipulation.cc
> (ipa_param_body_adjustments::modify_expression): Add assert checking
> sizes of a VIEW_CONVERT_EXPR will match.
> (ipa_param_body_adjustments::modify_assignment): Likewise.
>
> gcc/testsuite/ChangeLog:
>
> 2023-02-02 Martin Jambor <mjam...@suse.cz>
>
> PR ipa/108384
> * gcc.dg/ipa/pr108384.c: New test.
> ---
> gcc/ipa-param-manipulation.cc | 4 ++
> gcc/ipa-sra.cc | 66 ++++++++++++++++++++---------
> gcc/testsuite/gcc.dg/ipa/pr108384.c | 25 +++++++++++
> 3 files changed, 76 insertions(+), 19 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr108384.c
>
> diff --git a/gcc/ipa-param-manipulation.cc b/gcc/ipa-param-manipulation.cc
> index 1de9ca2ceb8..42488ee09c3 100644
> --- a/gcc/ipa-param-manipulation.cc
> +++ b/gcc/ipa-param-manipulation.cc
> @@ -1857,6 +1857,8 @@ ipa_param_body_adjustments::modify_expression (tree
> *expr_p, bool convert)
> if (convert && !useless_type_conversion_p (TREE_TYPE (expr),
> TREE_TYPE (repl)))
> {
> + gcc_checking_assert (tree_to_shwi (TYPE_SIZE (TREE_TYPE (expr)))
> + == tree_to_shwi (TYPE_SIZE (TREE_TYPE (repl))));
> tree vce = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (expr), repl);
> *expr_p = vce;
> }
> @@ -1900,6 +1902,8 @@ ipa_param_body_adjustments::modify_assignment (gimple
> *stmt,
> }
> else
> {
> + gcc_checking_assert (tree_to_shwi (TYPE_SIZE (TREE_TYPE (*lhs_p)))
> + == tree_to_shwi (TYPE_SIZE (TREE_TYPE
> (*rhs_p))));
> tree new_rhs = fold_build1_loc (gimple_location (stmt),
> VIEW_CONVERT_EXPR, TREE_TYPE
> (*lhs_p),
> *rhs_p);
> diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc
> index 81b75910db1..7a2b4dc8608 100644
> --- a/gcc/ipa-sra.cc
> +++ b/gcc/ipa-sra.cc
> @@ -3989,9 +3989,7 @@ push_param_adjustments_for_index (isra_func_summary
> *ifs, unsigned base_index,
> {
> ipa_argagg_value_list avl (ipcp_ts);
> tree value = avl.get_value (base_index, pa->unit_offset);
> - if (value
> - && (tree_to_uhwi (TYPE_SIZE (TREE_TYPE (value))) / BITS_PER_UNIT
> - == pa->unit_size))
> + if (value)
> {
> if (dump_file)
> fprintf (dump_file, " - omitting component at byte "
> @@ -4130,6 +4128,22 @@ process_isra_node_results (cgraph_node *node,
> callers.release ();
> }
>
> +/* If INDICES is not empty, dump a combination of NODE's dump_name and MSG
> + followed by the list of numbers in INDICES. */
> +
> +static void
> +dump_list_of_param_indices (const cgraph_node *node, const char* msg,
> + const vec<unsigned> &indices)
> +{
> + if (indices.is_empty ())
> + return;
> + fprintf (dump_file, "The following parameters of %s %s:", node->dump_name
> (),
> + msg);
> + for (unsigned i : indices)
> + fprintf (dump_file, " %u", i);
> + fprintf (dump_file, "\n");
> +}
> +
> /* Check which parameters of NODE described by IFS have survived until
> IPA-SRA
> and disable transformations for those which have not or which should not
> transformed because the associated debug counter reached its limit.
> Return
> @@ -4153,6 +4167,7 @@ adjust_parameter_descriptions (cgraph_node *node,
> isra_func_summary *ifs)
> check_surviving = true;
> cinfo->param_adjustments->get_surviving_params (&surviving_params);
> }
> + ipcp_transformation *ipcp_ts = ipcp_get_transformation_summary (node);
> auto_vec <unsigned> dump_dead_indices;
> auto_vec <unsigned> dump_bad_cond_indices;
> for (unsigned i = 0; i < len; i++)
> @@ -4202,27 +4217,40 @@ adjust_parameter_descriptions (cgraph_node *node,
> isra_func_summary *ifs)
> if (size_would_violate_limit_p (desc, desc->size_reached))
> desc->split_candidate = false;
> }
> +
> + /* Avoid ICEs on size-mismatched VIEW_CONVERT_EXPRs when callers and
> + callees don't agree on types in aggregates and we try to do both
> + IPA-CP and IPA-SRA. */
> + if (ipcp_ts && desc->split_candidate)
> + {
> + ipa_argagg_value_list avl (ipcp_ts);
> + for (const param_access *pa : desc->accesses)
> + {
> + if (!pa->certain)
> + continue;
> + tree value = avl.get_value (i, pa->unit_offset);
> + if (value
> + && ((tree_to_uhwi (TYPE_SIZE (TREE_TYPE (value)))
> + / BITS_PER_UNIT)
> + != pa->unit_size))
> + {
> + desc->split_candidate = false;
> + if (dump_file && (dump_flags & TDF_DETAILS))
> + dump_dead_indices.safe_push (i);
> + break;
> + }
> + }
> + }
> +
> if (desc->locally_unused || desc->split_candidate)
> ret = false;
> }
> }
>
> - if (!dump_dead_indices.is_empty ())
> - {
> - fprintf (dump_file, "The following parameters of %s are dead on
> arrival:",
> - node->dump_name ());
> - for (unsigned i : dump_dead_indices)
> - fprintf (dump_file, " %u", i);
> - fprintf (dump_file, "\n");
> - }
> - if (!dump_bad_cond_indices.is_empty ())
> - {
> - fprintf (dump_file, "The following parameters of %s are not safe to "
> - "derefernce in all callers:", node->dump_name ());
> - for (unsigned i : dump_bad_cond_indices)
> - fprintf (dump_file, " %u", i);
> - fprintf (dump_file, "\n");
> - }
> + dump_list_of_param_indices (node, "are dead on arrival or have a type "
> + "mismatch with IPA-CP", dump_dead_indices);
> + dump_list_of_param_indices (node, "are not safe to derefernce in all
> callers",
> + dump_bad_cond_indices);
>
> return ret;
> }
> diff --git a/gcc/testsuite/gcc.dg/ipa/pr108384.c
> b/gcc/testsuite/gcc.dg/ipa/pr108384.c
> new file mode 100644
> index 00000000000..2b714aa78b1
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/pr108384.c
> @@ -0,0 +1,25 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O3" } */
> +
> +struct S0 {
> + int f0;
> + short f1;
> + unsigned f2 : 7;
> + short f3;
> +} func_2_l_27;
> +int *g_389;
> +int safe_sub_func_int16_t_s_s(void);
> +void safe_lshift_func_uint8_t_u_s(int);
> +void func_23(struct S0 p_24, struct S0 p_25) {
> + int *l_1051 = g_389;
> + if (safe_sub_func_int16_t_s_s())
> + for (;;)
> + safe_lshift_func_uint8_t_u_s(p_24.f1);
> + *l_1051 = p_25.f0;
> +}
> +void func_2(void) {
> + struct S0 l_26[2];
> + l_26[1].f0 = 4;
> + ((long long*)&l_26)[2] = 25770065925;
> + func_23(l_26[1], func_2_l_27);
> +}
> --
> 2.39.0
>
