Fix tree-opt/110252: wrong code due to phiopt using flow sensitive info during match

  Match will query ranger via tree_nonzero_bits/get_nonzero_bits for 2 and 3rd
operand of the COND_EXPR and phiopt tries to do create the COND_EXPR even if we moving
one statement. That one statement could have some flow sensitive information on it
based on the condition that is for the COND_EXPR but that might create wrong code
if the statement was moved out.

So the solution here is to temporarily remove the flow sensitive info of the statements
phiopt might move and try match only then. This is done via the new class auto_flow_sensitive.

OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

	PR tree-optimization/110252

gcc/ChangeLog:

	* tree-ssa-phiopt.cc (match_simplify_replacement): Temporarily
	remove the flow sensitive info on the two statements that might
	be moved.
	* tree-ssanames.cc (auto_flow_sensitive::auto_flow_sensitive): New method.
	(auto_flow_sensitive::~auto_flow_sensitive): New method.
	* tree-ssanames.h (class auto_flow_sensitive): New class.

gcc/testsuite/ChangeLog:

	* gcc.c-torture/execute/pr110252.c: New test.
	* gcc.c-torture/execute/pr110252-1.c: New test.
	* gcc.dg/tree-ssa/phi-opt-25b.c: Updated as
	__builtin_parity loses the nonzerobits info.
---
 .../gcc.c-torture/execute/pr110252-1.c        | 15 +++++++
 .../gcc.c-torture/execute/pr110252.c          | 10 +++++
 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-25b.c   |  6 +--
 gcc/tree-ssa-phiopt.cc                        | 12 ++++--
 gcc/tree-ssanames.cc                          | 40 +++++++++++++++++++
 gcc/tree-ssanames.h                           | 21 ++++++++++
 6 files changed, 97 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr110252-1.c
 create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr110252.c
  

On Thu, Jun 15, 2023 at 8:18 AM Andrew Pinski via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Match will query ranger via tree_nonzero_bits/get_nonzero_bits for 2 and 3rd
> operand of the COND_EXPR and phiopt tries to do create the COND_EXPR even if we moving
> one statement. That one statement could have some flow sensitive information on it
> based on the condition that is for the COND_EXPR but that might create wrong code
> if the statement was moved out.
>
> So the solution here is to temporarily remove the flow sensitive info of the statements
> phiopt might move and try match only then. This is done via the new class auto_flow_sensitive.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

Ah, looks like I forgot about pt->null in the approach used by
maybe_fold_comparisons_from_match_pd.
That also has much less control over which stmts are possibly under
conditional control
so your approach might be simpler for phiopt.  Note the flow-sensitive
info is on SSA defs
and some stmts have multiple of those so I think the auto_ class
should construct on SSA defs
instead.  Can you make it a non-auto (POD?) class so I can possibly re-use it in
the maybe_fold_comparisons_from_match_pd machinery though?  Like

class flow_sensitive_info_storage
{
public:
  save (tree);
  save_and_clear (tree);
  restore (tree);
private:
  ... members ...
};

?


>         PR tree-optimization/110252
>
> gcc/ChangeLog:
>
>         * tree-ssa-phiopt.cc (match_simplify_replacement): Temporarily
>         remove the flow sensitive info on the two statements that might
>         be moved.
>         * tree-ssanames.cc (auto_flow_sensitive::auto_flow_sensitive): New method.
>         (auto_flow_sensitive::~auto_flow_sensitive): New method.
>         * tree-ssanames.h (class auto_flow_sensitive): New class.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.c-torture/execute/pr110252.c: New test.
>         * gcc.c-torture/execute/pr110252-1.c: New test.
>         * gcc.dg/tree-ssa/phi-opt-25b.c: Updated as
>         __builtin_parity loses the nonzerobits info.
> ---
>  .../gcc.c-torture/execute/pr110252-1.c        | 15 +++++++
>  .../gcc.c-torture/execute/pr110252.c          | 10 +++++
>  gcc/testsuite/gcc.dg/tree-ssa/phi-opt-25b.c   |  6 +--
>  gcc/tree-ssa-phiopt.cc                        | 12 ++++--
>  gcc/tree-ssanames.cc                          | 40 +++++++++++++++++++
>  gcc/tree-ssanames.h                           | 21 ++++++++++
>  6 files changed, 97 insertions(+), 7 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr110252-1.c
>  create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr110252.c
>
> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr110252-1.c b/gcc/testsuite/gcc.c-torture/execute/pr110252-1.c
> new file mode 100644
> index 00000000000..4ae93ca0647
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr110252-1.c
> @@ -0,0 +1,15 @@
> +/* This is reduced from sel-sched.cc which was noticed was being miscompiled too. */
> +int g(int min_need_stall) __attribute__((__noipa__));
> +int g(int min_need_stall)
> +{
> +  return  min_need_stall < 0 ? 1 : ((min_need_stall) < (1) ? (min_need_stall) : (1));
> +}
> +int main(void)
> +{
> +  for(int i = -100; i <= 100; i++)
> +    {
> +      int t = g(i);
> +      if (t != (i!=0))
> +        __builtin_abort();
> +    }
> +}
> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr110252.c b/gcc/testsuite/gcc.c-torture/execute/pr110252.c
> new file mode 100644
> index 00000000000..7f1a7dbf134
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr110252.c
> @@ -0,0 +1,10 @@
> +signed char f() __attribute__((__noipa__));
> +signed char f() { return 0; }
> +int main()
> +{
> +  int g = f() - 1;
> +  int e = g < 0 ? 1 : ((g >> (8-2))!=0);
> +  asm("":"+r"(e));
> +  if (e != 1)
> +    __builtin_abort();
> +}
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-25b.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-25b.c
> index 7298da0c96e..0fd9b004a03 100644
> --- a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-25b.c
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-25b.c
> @@ -65,8 +65,6 @@ int test_popcountll(unsigned long long x, unsigned long long y)
>    return x ? __builtin_popcountll(y) : 0;
>  }
>
> -/* 3 types of functions (not including parity), each with 3 types and there are 2 goto each */
> -/* { dg-final { scan-tree-dump-times "goto " 18 "optimized" } } */
> +/* 4 types of functions, each with 3 types and there are 2 goto each */
> +/* { dg-final { scan-tree-dump-times "goto " 24 "optimized" } } */
>  /* { dg-final { scan-tree-dump-times "x_..D. != 0" 12 "optimized" } } */
> -/* parity case will be optimized to x!=0 & parity(y) . */
> -/* { dg-final { scan-tree-dump-times " & " 3 "optimized" } } */
> diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> index d9a1153086f..5f871e019e4 100644
> --- a/gcc/tree-ssa-phiopt.cc
> +++ b/gcc/tree-ssa-phiopt.cc
> @@ -782,9 +782,15 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
>      }
>
>    tree type = TREE_TYPE (gimple_phi_result (phi));
> -  result = gimple_simplify_phiopt (early_p, type, stmt,
> -                                  arg_true, arg_false,
> -                                  &seq);
> +  {
> +    auto_flow_sensitive s1(stmt_to_move);
> +    auto_flow_sensitive s_alt(stmt_to_move_alt);
> +
> +    result = gimple_simplify_phiopt (early_p, type, stmt,
> +                                    arg_true, arg_false,
> +                                    &seq);
> +  }
> +
>    if (!result)
>      return false;
>
> diff --git a/gcc/tree-ssanames.cc b/gcc/tree-ssanames.cc
> index 5fdb6a37e9f..765d343ba49 100644
> --- a/gcc/tree-ssanames.cc
> +++ b/gcc/tree-ssanames.cc
> @@ -812,6 +812,46 @@ reset_flow_sensitive_info_in_bb (basic_block bb)
>      }
>  }
>
> +/* Constructor for auto_flow_sensitive. Saves
> +   off the ssa name's flow sensitive information
> +   that was defined by gimple statement S and
> +   resets it to be non-flow based ones. */
> +auto_flow_sensitive::auto_flow_sensitive (gimple *s)
> +{
> +  if (!s)
> +    return;
> +
> +  name = gimple_get_lhs (s);
> +
> +  if (!POINTER_TYPE_P (TREE_TYPE (name)))
> +    {
> +      range_info = SSA_NAME_RANGE_INFO (name);
> +      SSA_NAME_RANGE_INFO (name) = NULL;
> +    }
> +  else if (SSA_NAME_PTR_INFO (name))
> +    {
> +      get_ptr_info_alignment (SSA_NAME_PTR_INFO (name), &align, &misalign);
> +      null = SSA_NAME_PTR_INFO (name)->pt.null;
> +    }
> +}
> +
> +/* Deconstructor, restores the flow sensitive information
> +   for the SSA name.  */
> +auto_flow_sensitive::~auto_flow_sensitive ()
> +{
> +  if (!name)
> +    return;
> +
> +  if (!POINTER_TYPE_P (TREE_TYPE (name)))
> +    SSA_NAME_RANGE_INFO (name) = range_info;
> +  else if (SSA_NAME_PTR_INFO (name))
> +    {
> +      if (align)
> +       set_ptr_info_alignment (SSA_NAME_PTR_INFO (name), align, misalign);
> +      SSA_NAME_PTR_INFO (name)->pt.null = null;
> +    }
> +}
> +
>  /* Release all the SSA_NAMEs created by STMT.  */
>
>  void
> diff --git a/gcc/tree-ssanames.h b/gcc/tree-ssanames.h
> index f3fa609208a..28b83b779fa 100644
> --- a/gcc/tree-ssanames.h
> +++ b/gcc/tree-ssanames.h
> @@ -137,4 +137,25 @@ make_temp_ssa_name (tree type, gimple *stmt, const char *name)
>  }
>
>
> +/* RAII style class to temporarily remove flow sensitive
> +   from a ssa name defined by a gimple statement.
> +   An usage example is inside phiopt, where using match-and-simplify
> +   but since the statement is from inside a conditional bb, the
> +   flow sensitive information can't be used.  */
> +class auto_flow_sensitive
> +{
> +public:
> +  auto_flow_sensitive (gimple *s);
> +  ~auto_flow_sensitive ();
> +private:
> +  tree name = nullptr;
> +  /* The range info for non pointers */
> +  vrange_storage *range_info;
> +  /* Flow sensitive pointer information. */
> +  unsigned int align = 0;
> +  unsigned int misalign = 0;
> +  bool null = false;
> +};
> +
> +
>  #endif /* GCC_TREE_SSANAMES_H */
> --
> 2.31.1
>