phiopt: Fix VCE moving by rewriting it into cast [PR116098]

Phiopt match_and_simplify might move a well defined VCE assign statement
from being conditional to being uncondtitional; that VCE might no longer
being defined. It will need a rewrite into a cast instead.

This adds the rewriting code to move_stmt for the VCE case.
This is enough to fix the issue at hand. It should also be using rewrite_to_defined_overflow
but first I need to move the check to see a rewrite is needed into its own function
and that is causing issues (see https://gcc.gnu.org/pipermail/gcc-patches/2024-September/663938.html).
Plus this version is easiest to backport.

Bootstrapped and tested on x86_64-linux-gnu.

	PR tree-optimization/116098

gcc/ChangeLog:

	* tree-ssa-phiopt.cc (move_stmt): Rewrite VCEs from integer to integer
	types to case.

gcc/testsuite/ChangeLog:

	* c-c++-common/torture/pr116098-2.c: New test.
	* g++.dg/torture/pr116098-1.C: New test.

Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
---
 .../c-c++-common/torture/pr116098-2.c         | 46 +++++++++++++++++++
 gcc/testsuite/g++.dg/torture/pr116098-1.C     | 33 +++++++++++++
 gcc/tree-ssa-phiopt.cc                        | 28 ++++++++++-
 3 files changed, 106 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/c-c++-common/torture/pr116098-2.c
 create mode 100644 gcc/testsuite/g++.dg/torture/pr116098-1.C

On Wed, Oct 2, 2024 at 1:11 AM Andrew Pinski <quic_apinski@quicinc.com> wrote:
>
> Phiopt match_and_simplify might move a well defined VCE assign statement
> from being conditional to being uncondtitional; that VCE might no longer
> being defined. It will need a rewrite into a cast instead.
>
> This adds the rewriting code to move_stmt for the VCE case.

Indeed.

> This is enough to fix the issue at hand. It should also be using rewrite_to_defined_overflow
> but first I need to move the check to see a rewrite is needed into its own function
> and that is causing issues (see https://gcc.gnu.org/pipermail/gcc-patches/2024-September/663938.html).
> Plus this version is easiest to backport.

OK.

Thanks,
Richard.

> Bootstrapped and tested on x86_64-linux-gnu.
>
>         PR tree-optimization/116098
>
> gcc/ChangeLog:
>
>         * tree-ssa-phiopt.cc (move_stmt): Rewrite VCEs from integer to integer
>         types to case.
>
> gcc/testsuite/ChangeLog:
>
>         * c-c++-common/torture/pr116098-2.c: New test.
>         * g++.dg/torture/pr116098-1.C: New test.
>
> Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
> ---
>  .../c-c++-common/torture/pr116098-2.c         | 46 +++++++++++++++++++
>  gcc/testsuite/g++.dg/torture/pr116098-1.C     | 33 +++++++++++++
>  gcc/tree-ssa-phiopt.cc                        | 28 ++++++++++-
>  3 files changed, 106 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/c-c++-common/torture/pr116098-2.c
>  create mode 100644 gcc/testsuite/g++.dg/torture/pr116098-1.C
>
> diff --git a/gcc/testsuite/c-c++-common/torture/pr116098-2.c b/gcc/testsuite/c-c++-common/torture/pr116098-2.c
> new file mode 100644
> index 00000000000..614ed049171
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/torture/pr116098-2.c
> @@ -0,0 +1,46 @@
> +/* { dg-do run } */
> +/* PR tree-optimization/116098 */
> +
> +
> +#include <stdbool.h>
> +
> +struct Value {
> +    int type;
> +    union {
> +        bool boolean;
> +        long long t;
> +    };
> +};
> +
> +static struct Value s_item_mem;
> +
> +/* truthy was being miscompiled for the value.type==2 case,
> +   because we would have a VCE from unsigned char to bool
> +   that went from being conditional in the value.type==1 case
> +   to unconditional when `value.type!=0`.
> +   The move of the VCE from conditional to unconditional,
> +   needs to changed into a convert (NOP_EXPR). */
> +static bool truthy(void) __attribute__((noipa));
> +static bool
> +truthy(void)
> +{
> +    struct Value value = s_item_mem;
> +    if (value.type == 0)
> +      return 0;
> +    if (value.type == 1)
> +      return value.boolean;
> +    return 1;
> +}
> +
> +int
> +main(void)
> +{
> +    s_item_mem.type = 2;
> +    s_item_mem.t = -1;
> +    bool b1 = !truthy();
> +    s_item_mem.type = 1;
> +    s_item_mem.boolean = b1;
> +    bool b = truthy();
> +    if (b1 != b)  __builtin_abort();
> +    if (b) __builtin_abort();
> +}
> diff --git a/gcc/testsuite/g++.dg/torture/pr116098-1.C b/gcc/testsuite/g++.dg/torture/pr116098-1.C
> new file mode 100644
> index 00000000000..90e44a6eeed
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/torture/pr116098-1.C
> @@ -0,0 +1,33 @@
> +// { dg-do run }
> +/* PR tree-optimization/116098 */
> +
> +
> +static bool truthy(int type, unsigned char data) __attribute__((noipa));
> +/* truthy was being miscompiled for the type==2 case,
> +   because we would have a VCE from unsigned char to bool
> +   that went from being conditional in the type==1 case
> +   to unconditional when `type!=0`.
> +   The move of the VCE from conditional to unconditional,
> +   needs to changed into a convert (NOP_EXPR). */
> +
> +static bool truthy(void) __attribute__((noipa));
> +static bool
> +truthy(int type, unsigned char data)
> +{
> +    if (type == 0)
> +      return 0;
> +    if (type == 1)
> +      /* Emulate what SRA does, so this can be
> +        tested without depending on SRA. */
> +      return __builtin_bit_cast (bool, data);
> +    return 1;
> +}
> +
> +int
> +main(void)
> +{
> +    bool b1 = !truthy(2, -1);
> +    bool b = truthy(1, b1);
> +    if (b1 != b)  __builtin_abort();
> +    if (b) __builtin_abort();
> +}
> diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> index bd7f9607eb9..43b65b362a3 100644
> --- a/gcc/tree-ssa-phiopt.cc
> +++ b/gcc/tree-ssa-phiopt.cc
> @@ -742,7 +742,8 @@ empty_bb_or_one_feeding_into_p (basic_block bb,
>  }
>
>  /* Move STMT to before GSI and insert its defining
> -   name into INSERTED_EXPRS bitmap. */
> +   name into INSERTED_EXPRS bitmap.
> +   Also rewrite its if it might be undefined when unconditionalized.  */
>  static void
>  move_stmt (gimple *stmt, gimple_stmt_iterator *gsi, auto_bitmap &inserted_exprs)
>  {
> @@ -761,6 +762,31 @@ move_stmt (gimple *stmt, gimple_stmt_iterator *gsi, auto_bitmap &inserted_exprs)
>    gimple_stmt_iterator gsi1 = gsi_for_stmt (stmt);
>    gsi_move_before (&gsi1, gsi);
>    reset_flow_sensitive_info (name);
> +
> +  /* Rewrite some code which might be undefined when
> +     unconditionalized. */
> +  if (gimple_assign_single_p (stmt))
> +    {
> +      tree rhs = gimple_assign_rhs1 (stmt);
> +      /* VCE from integral types to another integral types but with
> +        different precisions need to be changed into casts
> +        to be well defined when unconditional. */
> +      if (gimple_assign_rhs_code (stmt) == VIEW_CONVERT_EXPR
> +         && INTEGRAL_TYPE_P (TREE_TYPE (name))
> +         && INTEGRAL_TYPE_P (TREE_TYPE (TREE_OPERAND (rhs, 0))))
> +       {
> +         if (dump_file && (dump_flags & TDF_DETAILS))
> +           {
> +             fprintf (dump_file, "rewriting stmt with maybe undefined VCE ");
> +             print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
> +           }
> +         tree new_rhs = TREE_OPERAND (rhs, 0);
> +         gcc_assert (is_gimple_val (new_rhs));
> +         gimple_assign_set_rhs_code (stmt, NOP_EXPR);
> +         gimple_assign_set_rhs1 (stmt, new_rhs);
> +         update_stmt (stmt);
> +       }
> +    }
>  }
>
>  /* RAII style class to temporarily remove flow sensitive
> --
> 2.34.1
>