Re: [PATCH] ipa: Avoid invalid gimple when IPA-CP and IPA-SRA disagree on types (108384)

[Richard Biener <richard.guenther@gmail.com>]

On Thu, Feb 2, 2023 at 5:20 PM Martin Jambor <mjambor@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  <mjambor@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  <mjambor@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
>