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

[Richard Biener <richard.guenther@gmail.com>]

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
>