On Wed, May 7, 2025 at 4:10 AM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> On Wed, May 7, 2025 at 3:55 AM Andrew Pinski <quic_apin...@quicinc.com> wrote:
> >
> > Like the patch to phiopt (r15-4033-g1f619fe25925a5f7), this adds rewriting
> > of VCE to gimple_with_undefined_signed_overflow/rewrite_to_defined_overflow.
> > In the case of moving VCE of a bool from being conditional to unconditional,
> > it needs to be rewritten to not to use VCE but a normal cast. pr120122-1.c
> > is
> > an example of where LIM needs this rewriting.
> >
> > This also renames gimple_with_undefined_signed_overflow to
> > gimple_needing_rewrite_undefined
> > and rewrite_to_defined_overflow to rewrite_to_defined_unconditional as they
> > will be doing
> > more than just handling signed overflow.
> >
> > Bootstrappd and tested on x86_64-linux-gnu.
> >
> > PR tree-optimization/120122
> > PR tree-optimization/116939
> >
> > gcc/ChangeLog:
> >
> > * gimple-fold.h (gimple_with_undefined_signed_overflow): Rename to
> > ..
> > (rewrite_to_defined_overflow): This.
> > (gimple_needing_rewrite_undefined): Rename to ...
> > (rewrite_to_defined_unconditional): this.
> > * gimple-fold.cc (gimple_with_undefined_signed_overflow): Rename to
> > ...
> > (gimple_needing_rewrite_undefined): This. Return true for VCE with
> > integral
> > types.
> > (rewrite_to_defined_overflow): Rename to ...
> > (rewrite_to_defined_unconditional): This. Handle VCE rewriting to a
> > cast.
> > * tree-if-conv.cc:
> > s/gimple_with_undefined_signed_overflow/gimple_needing_rewrite_undefined/
> > s/rewrite_to_defined_overflow/rewrite_to_defined_unconditional.
> > * tree-scalar-evolution.cc: Likewise
> > * tree-ssa-ifcombine.cc: Likewise.
> > * tree-ssa-loop-im.cc: Likewise.
> > * tree-ssa-loop-split.cc: Likewise.
> > * tree-ssa-reassoc.cc: Likewise.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.dg/torture/pr120122-1.c: New test.
> >
> > Signed-off-by: Andrew Pinski <quic_apin...@quicinc.com>
> > ---
> > gcc/gimple-fold.cc | 51 +++++++++++++++++------
> > gcc/gimple-fold.h | 6 +--
> > gcc/testsuite/gcc.dg/torture/pr120122-1.c | 51 +++++++++++++++++++++++
> > gcc/tree-if-conv.cc | 6 +--
> > gcc/tree-scalar-evolution.cc | 4 +-
> > gcc/tree-ssa-ifcombine.cc | 4 +-
> > gcc/tree-ssa-loop-im.cc | 4 +-
> > gcc/tree-ssa-loop-split.cc | 4 +-
> > gcc/tree-ssa-reassoc.cc | 4 +-
> > 9 files changed, 106 insertions(+), 28 deletions(-)
> > create mode 100644 gcc/testsuite/gcc.dg/torture/pr120122-1.c
> >
> > diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
> > index c060ef81a42..4f45aeb7ff8 100644
> > --- a/gcc/gimple-fold.cc
> > +++ b/gcc/gimple-fold.cc
> > @@ -10588,10 +10588,12 @@ arith_code_with_undefined_signed_overflow
> > (tree_code code)
> >
> > /* Return true if STMT has an operation that operates on a signed
> > integer types involves undefined behavior on overflow and the
> > - operation can be expressed with unsigned arithmetic. */
> > + operation can be expressed with unsigned arithmetic.
> > + Also returns true if STMT is a VCE that needs to be rewritten
> > + if moved to be executed unconditionally. */
> >
> > bool
> > -gimple_with_undefined_signed_overflow (gimple *stmt)
> > +gimple_needing_rewrite_undefined (gimple *stmt)
> > {
> > if (!is_gimple_assign (stmt))
> > return false;
> > @@ -10602,6 +10604,14 @@ gimple_with_undefined_signed_overflow (gimple
> > *stmt)
> > if (!INTEGRAL_TYPE_P (lhs_type)
> > && !POINTER_TYPE_P (lhs_type))
> > return false;
> > + 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. */
> > + if (gimple_assign_rhs_code (stmt) == VIEW_CONVERT_EXPR
> > + && INTEGRAL_TYPE_P (TREE_TYPE (TREE_OPERAND (rhs, 0)))
>
> So this does not perform the precision check and IMO we should never end
> up with a V_C_E from a lower-precision operand to a higher precision. So
> does it work with an additional
>
> && TYPE_PRECISION (TREE_TYPE (TREE_OPERAND (rhs, 0))
> > TYPE_PRECISION (lhs_type)
>
> check?
Yes. I added that check and retested pr120122-1.c and that testcase
does not fail.
I am doing a full bootstrap/test of the patch and will submit a new
version once it finishes.
Thanks,
Andrew Pinski
>
> > + && is_gimple_val (TREE_OPERAND (rhs, 0)))
> > + return true;
> > if (!TYPE_OVERFLOW_UNDEFINED (lhs_type))
> > return false;
> > if (!arith_code_with_undefined_signed_overflow
> > @@ -10621,19 +10631,36 @@ gimple_with_undefined_signed_overflow (gimple
> > *stmt)
> > contain a modified form of STMT itself. */
> >
> > static gimple_seq
> > -rewrite_to_defined_overflow (gimple_stmt_iterator *gsi, gimple *stmt,
> > - bool in_place)
> > +rewrite_to_defined_unconditional (gimple_stmt_iterator *gsi, gimple *stmt,
> > + bool in_place)
> > {
> > if (dump_file && (dump_flags & TDF_DETAILS))
> > {
> > - fprintf (dump_file, "rewriting stmt with undefined signed "
> > - "overflow ");
> > + fprintf (dump_file, "rewriting stmt for being uncondtional defined");
> > print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
> > }
> > -
> > + gimple_seq stmts = NULL;
> > + /* VCE from integral types to another integral types but with
> > + different precisions need to be changed into casts
> > + to be well defined. */
> > + if (gimple_assign_rhs_code (stmt) == VIEW_CONVERT_EXPR)
> > + {
> > + tree rhs = gimple_assign_rhs1 (stmt);
> > + 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);
> > + if (in_place)
> > + update_stmt (stmt);
> > + else
> > + {
> > + gimple_set_modified (stmt, true);
> > + gimple_seq_add_stmt (&stmts, stmt);
> > + }
> > + return stmts;
> > + }
> > tree lhs = gimple_assign_lhs (stmt);
> > tree type = unsigned_type_for (TREE_TYPE (lhs));
> > - gimple_seq stmts = NULL;
> > if (gimple_assign_rhs_code (stmt) == ABS_EXPR)
> > gimple_assign_set_rhs_code (stmt, ABSU_EXPR);
> > else
> > @@ -10668,15 +10695,15 @@ rewrite_to_defined_overflow (gimple_stmt_iterator
> > *gsi, gimple *stmt,
> > }
> >
> > void
> > -rewrite_to_defined_overflow (gimple_stmt_iterator *gsi)
> > +rewrite_to_defined_unconditional (gimple_stmt_iterator *gsi)
> > {
> > - rewrite_to_defined_overflow (gsi, gsi_stmt (*gsi), true);
> > + rewrite_to_defined_unconditional (gsi, gsi_stmt (*gsi), true);
> > }
> >
> > gimple_seq
> > -rewrite_to_defined_overflow (gimple *stmt)
> > +rewrite_to_defined_unconditional (gimple *stmt)
> > {
> > - return rewrite_to_defined_overflow (nullptr, stmt, false);
> > + return rewrite_to_defined_unconditional (nullptr, stmt, false);
> > }
> >
> > /* The valueization hook we use for the gimple_build API simplification.
> > diff --git a/gcc/gimple-fold.h b/gcc/gimple-fold.h
> > index 5fcfdcda81b..afecbb8ceef 100644
> > --- a/gcc/gimple-fold.h
> > +++ b/gcc/gimple-fold.h
> > @@ -59,9 +59,9 @@ extern tree gimple_get_virt_method_for_vtable
> > (HOST_WIDE_INT, tree,
> > extern tree gimple_fold_indirect_ref (tree);
> > extern bool gimple_fold_builtin_sprintf (gimple_stmt_iterator *);
> > extern bool gimple_fold_builtin_snprintf (gimple_stmt_iterator *);
> > -extern bool gimple_with_undefined_signed_overflow (gimple *);
> > -extern void rewrite_to_defined_overflow (gimple_stmt_iterator *);
> > -extern gimple_seq rewrite_to_defined_overflow (gimple *);
> > +extern bool gimple_needing_rewrite_undefined (gimple *);
> > +extern void rewrite_to_defined_unconditional (gimple_stmt_iterator *);
> > +extern gimple_seq rewrite_to_defined_unconditional (gimple *);
> > extern void replace_call_with_value (gimple_stmt_iterator *, tree);
> > extern tree tree_vec_extract (gimple_stmt_iterator *, tree, tree, tree,
> > tree);
> > extern void gsi_replace_with_seq_vops (gimple_stmt_iterator *, gimple_seq);
> > diff --git a/gcc/testsuite/gcc.dg/torture/pr120122-1.c
> > b/gcc/testsuite/gcc.dg/torture/pr120122-1.c
> > new file mode 100644
> > index 00000000000..dde41d8ec06
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/torture/pr120122-1.c
> > @@ -0,0 +1,51 @@
> > +/* { dg-do run } */
> > +/* PR tree-optimization/120122 */
> > +
> > +#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)
> > +{
> > + bool tt = false;
> > + for(int i = 0; i < 10; i++)
> > + {
> > + struct Value value = s_item_mem;
> > + if (value.type == 0)
> > + tt = tt | 0;
> > + else if (value.type == 1)
> > + tt = tt | value.boolean;
> > + else
> > + tt = tt | 1;
> > + }
> > + return tt;
> > +}
> > +
> > +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/tree-if-conv.cc b/gcc/tree-if-conv.cc
> > index fe8aee057b3..636361e7c36 100644
> > --- a/gcc/tree-if-conv.cc
> > +++ b/gcc/tree-if-conv.cc
> > @@ -1066,7 +1066,7 @@ if_convertible_gimple_assign_stmt_p (gimple *stmt,
> > fprintf (dump_file, "tree could trap...\n");
> > return false;
> > }
> > - else if (gimple_with_undefined_signed_overflow (stmt))
> > + else if (gimple_needing_rewrite_undefined (stmt))
> > /* We have to rewrite stmts with undefined overflow. */
> > need_to_rewrite_undefined = true;
> >
> > @@ -2881,8 +2881,8 @@ predicate_statements (loop_p loop)
> >
> > gsi_replace (&gsi, new_stmt, true);
> > }
> > - else if (gimple_with_undefined_signed_overflow (stmt))
> > - rewrite_to_defined_overflow (&gsi);
> > + else if (gimple_needing_rewrite_undefined (stmt))
> > + rewrite_to_defined_unconditional (&gsi);
> > else if (gimple_vdef (stmt))
> > {
> > tree lhs = gimple_assign_lhs (stmt);
> > diff --git a/gcc/tree-scalar-evolution.cc b/gcc/tree-scalar-evolution.cc
> > index 9d64d3a8721..43311e52f94 100644
> > --- a/gcc/tree-scalar-evolution.cc
> > +++ b/gcc/tree-scalar-evolution.cc
> > @@ -3932,8 +3932,8 @@ final_value_replacement_loop (class loop *loop)
> > gsi2 = gsi_start (stmts);
> > while (!gsi_end_p (gsi2))
> > {
> > - if (gimple_with_undefined_signed_overflow (gsi_stmt (gsi2)))
> > - rewrite_to_defined_overflow (&gsi2);
> > + if (gimple_needing_rewrite_undefined (gsi_stmt (gsi2)))
> > + rewrite_to_defined_unconditional (&gsi2);
> > gsi_next (&gsi2);
> > }
> > }
> > diff --git a/gcc/tree-ssa-ifcombine.cc b/gcc/tree-ssa-ifcombine.cc
> > index 19990d67202..1fff9234198 100644
> > --- a/gcc/tree-ssa-ifcombine.cc
> > +++ b/gcc/tree-ssa-ifcombine.cc
> > @@ -514,9 +514,9 @@ ifcombine_mark_ssa_name_walk (tree *t, int *, void
> > *data_)
> > static inline void
> > ifcombine_rewrite_to_defined_overflow (gimple_stmt_iterator gsi)
> > {
> > - if (!gimple_with_undefined_signed_overflow (gsi_stmt (gsi)))
> > + if (!gimple_needing_rewrite_undefined (gsi_stmt (gsi)))
> > return;
> > - rewrite_to_defined_overflow (&gsi);
> > + rewrite_to_defined_unconditional (&gsi);
> > }
> >
> >
> > diff --git a/gcc/tree-ssa-loop-im.cc b/gcc/tree-ssa-loop-im.cc
> > index ae2fd87d589..50d2ee9b99d 100644
> > --- a/gcc/tree-ssa-loop-im.cc
> > +++ b/gcc/tree-ssa-loop-im.cc
> > @@ -1419,11 +1419,11 @@ move_computations_worker (basic_block bb)
> > when the target loop header is executed and the stmt may
> > invoke undefined integer or pointer overflow rewrite it to
> > unsigned arithmetic. */
> > - if (gimple_with_undefined_signed_overflow (stmt)
> > + if (gimple_needing_rewrite_undefined (stmt)
> > && (!ALWAYS_EXECUTED_IN (bb)
> > || !(ALWAYS_EXECUTED_IN (bb) == level
> > || flow_loop_nested_p (ALWAYS_EXECUTED_IN (bb), level))))
> > - gsi_insert_seq_on_edge (e, rewrite_to_defined_overflow (stmt));
> > + gsi_insert_seq_on_edge (e, rewrite_to_defined_unconditional (stmt));
> > else
> > gsi_insert_on_edge (e, stmt);
> > }
> > diff --git a/gcc/tree-ssa-loop-split.cc b/gcc/tree-ssa-loop-split.cc
> > index 80f488a4f9a..492abf849b8 100644
> > --- a/gcc/tree-ssa-loop-split.cc
> > +++ b/gcc/tree-ssa-loop-split.cc
> > @@ -663,8 +663,8 @@ split_loop (class loop *loop1)
> > gsi = gsi_start (stmts2);
> > while (!gsi_end_p (gsi))
> > {
> > - if (gimple_with_undefined_signed_overflow (gsi_stmt
> > (gsi)))
> > - rewrite_to_defined_overflow (&gsi);
> > + if (gimple_needing_rewrite_undefined (gsi_stmt (gsi)))
> > + rewrite_to_defined_unconditional (&gsi);
> > gsi_next (&gsi);
> > }
> > }
> > diff --git a/gcc/tree-ssa-reassoc.cc b/gcc/tree-ssa-reassoc.cc
> > index 13bb85c4b71..3c38f3d7a19 100644
> > --- a/gcc/tree-ssa-reassoc.cc
> > +++ b/gcc/tree-ssa-reassoc.cc
> > @@ -2925,13 +2925,13 @@ update_range_test (struct range_entry *range,
> > struct range_entry *otherrange,
> > !gsi_end_p (gsi); gsi_next (&gsi))
> > {
> > gimple *stmt = gsi_stmt (gsi);
> > - if (gimple_with_undefined_signed_overflow (stmt))
> > + if (gimple_needing_rewrite_undefined (stmt))
> > {
> > gimple_stmt_iterator gsip = gsi;
> > gimple_stmt_iterator gsin = gsi;
> > gsi_prev (&gsip);
> > gsi_next (&gsin);
> > - rewrite_to_defined_overflow (&gsi);
> > + rewrite_to_defined_unconditional (&gsi);
> > unsigned uid = gimple_uid (stmt);
> > if (gsi_end_p (gsip))
> > gsip = gsi_after_labels (bb);
> > --
> > 2.34.1
> >
