> Am 31.08.2024 um 02:27 schrieb Andrew Pinski <quic_apin...@quicinc.com>:
>
> The heurstics that was added for PR71016, try to search to see
> if the conversion was being moved away from its definition. The problem
> is the heurstics would stop if there was a non GIMPLE_ASSIGN (and already
> ignores
> debug statements) and in this case we would have a GIMPLE_LABEL that was not
> being ignored. So we should need to ignore GIMPLE_NOP, GIMPLE_LABEL and
> GIMPLE_PREDICT.
> Note this is now similar to how gimple_empty_block_p behaves.
>
> Note this fixes the wrong code that was reported by moving the VCE
> (conversion) out before
> the phiopt/match could convert it into an bit_ior and move the VCE out with
> the VCE being
> conditionally valid.
>
> Bootstrapped and tested on x86_64-linux-gnu.
> Also built and tested for aarch64-linux-gnu.
Ok, also for the testcase adjustment
Richard
> PR tree-optimization/116098
>
> gcc/ChangeLog:
>
> * tree-ssa-phiopt.cc (factor_out_conditional_operation): Ignore
> nops, labels and predicts for heuristic for conversion with a constant.
>
> gcc/testsuite/ChangeLog:
>
> * c-c++-common/torture/pr116098-1.c: New test.
> * gcc.target/aarch64/csel-1.c: New test.
>
> Signed-off-by: Andrew Pinski <quic_apin...@quicinc.com>
> ---
> .../c-c++-common/torture/pr116098-1.c | 84 +++++++++++++++++++
> gcc/testsuite/gcc.target/aarch64/csel-1.c | 28 +++++++
> gcc/tree-ssa-phiopt.cc | 9 +-
> 3 files changed, 119 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/c-c++-common/torture/pr116098-1.c
> create mode 100644 gcc/testsuite/gcc.target/aarch64/csel-1.c
>
> diff --git a/gcc/testsuite/c-c++-common/torture/pr116098-1.c
> b/gcc/testsuite/c-c++-common/torture/pr116098-1.c
> new file mode 100644
> index 00000000000..b9d9a342305
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/torture/pr116098-1.c
> @@ -0,0 +1,84 @@
> +/* { dg-do run } */
> +/* PR tree-optimization/116098 */
> +/* truthy was being miscompiled where the VCE was not being pulled out
> + of the if statement by factor_out_conditional_operation before the rest of
> + phiopt would happen which assumed VCE would be correct. */
> +/* The unused label was causing truthy to have different code generation
> than truthy_1. */
> +
> +
> +#ifndef __cplusplus
> +#define bool _Bool
> +#endif
> +
> +enum ValueType {
> + VALUE_BOOLEAN,
> + VALUE_NUM,
> +};
> +
> +struct Value {
> + enum ValueType type;
> + union {
> + bool boolean;
> + int num;
> + };
> +};
> +
> +static struct Value s_value;
> +static bool s_b;
> +
> +
> +bool truthy_1(void) __attribute__((noinline));
> +bool
> +truthy_1(void)
> +{
> + struct Value value = s_value;
> + if (s_b) s_b = 0;
> + enum ValueType t = value.type;
> + if (t != VALUE_BOOLEAN)
> + return 1;
> + return value.boolean;
> +}
> +bool truthy(void) __attribute__((noinline));
> +bool
> +truthy(void)
> +{
> + struct Value value = s_value;
> + if (s_b) s_b = 0;
> + enum ValueType t = value.type;
> + if (t != VALUE_BOOLEAN)
> + return 1;
> + /* This unused label should not cause any difference in code generation. */
> +a: __attribute__((unused));
> + return value.boolean;
> +}
> +
> +int
> +main(void)
> +{
> + s_b = 0;
> + s_value = (struct Value) {
> + .type = VALUE_NUM,
> + .num = 2,
> + };
> + s_value = (struct Value) {
> + .type = VALUE_BOOLEAN,
> + .boolean = !truthy_1(),
> + };
> + bool b = truthy_1();
> + if (b)
> + __builtin_abort();
> +
> + s_b = 0;
> + s_value = (struct Value) {
> + .type = VALUE_NUM,
> + .num = 2,
> + };
> + s_value = (struct Value) {
> + .type = VALUE_BOOLEAN,
> + .boolean = !truthy(),
> + };
> + b = truthy();
> + if (b)
> + __builtin_abort();
> +}
> +
> diff --git a/gcc/testsuite/gcc.target/aarch64/csel-1.c
> b/gcc/testsuite/gcc.target/aarch64/csel-1.c
> new file mode 100644
> index 00000000000..a20d39ea375
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/csel-1.c
> @@ -0,0 +1,28 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +/* These 2 functions should be the same; even though there is a label in f1.
> + The label should not make a difference in code generation.
> + There sign extend should be removed as it is not needed. */
> +void f(int t, int a, short *b)
> +{
> + short t1 = 1;
> + if (a)
> + {
> + t1 = t;
> + }
> + *b = t1;
> +}
> +
> +void f1(int t, int a, short *b)
> +{
> + short t1 = 1;
> + if (a)
> + {
> + label1: __attribute__((unused))
> + t1 = t;
> + }
> + *b = t1;
> +}
> +
> +/* { dg-final { scan-assembler-not "sxth\t" } } */
> diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> index 9a009e187ee..271a5d51f09 100644
> --- a/gcc/tree-ssa-phiopt.cc
> +++ b/gcc/tree-ssa-phiopt.cc
> @@ -324,8 +324,13 @@ factor_out_conditional_operation (edge e0, edge e1, gphi
> *phi,
> gsi_prev_nondebug (&gsi);
> if (!gsi_end_p (gsi))
> {
> - if (gassign *assign
> - = dyn_cast <gassign *> (gsi_stmt (gsi)))
> + gimple *stmt = gsi_stmt (gsi);
> + /* Ignore nops, predicates and labels. */
> + if (gimple_code (stmt) == GIMPLE_NOP
> + || gimple_code (stmt) == GIMPLE_PREDICT
> + || gimple_code (stmt) == GIMPLE_LABEL)
> + ;
> + else if (gassign *assign = dyn_cast <gassign *> (stmt))
> {
> tree lhs = gimple_assign_lhs (assign);
> enum tree_code ass_code
> --
> 2.43.0
>
