Message ID | 20241001231054.1356313-1-quic_apinski@quicinc.com |
---|---|
State | New |
Headers | show |
Series | phiopt: Fix VCE moving by rewriting it into cast [PR116098] | expand |
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 >
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
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