[2/2] phiopt: Ignore some nop statements in heursics [PR116098]

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.

	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_apinski@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

> Am 31.08.2024 um 02:27 schrieb Andrew Pinski <quic_apinski@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_apinski@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
>