[PATCH 1/2] Add flow_sensitive_info_storage and use it in gimple-fold.

[PATCH 1/2] Add flow_sensitive_info_storage and use it in gimple-fold.

[Andrew Pinski]

This adds flow_sensitive_info_storage and uses it in
maybe_fold_comparisons_from_match_pd as mentioned in
https://gcc.gnu.org/pipermail/gcc-patches/2023-June/621817.html .
Since using it in maybe_fold_comparisons_from_match_pd was easy
and allowed me to test the storage earlier, I did it.

This also hides better how the flow sensitive information is
stored and only a single place needs to be updated if that
ever changes (again).

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

gcc/ChangeLog:

	* gimple-fold.cc (fosa_unwind): Replace `vrange_storage *`
	with flow_sensitive_info_storage.
	(follow_outer_ssa_edges): Update how to save off the flow
	sensitive info.
	(maybe_fold_comparisons_from_match_pd): Update restoring
	of flow sensitive info.
	* tree-ssanames.cc (flow_sensitive_info_storage::save): New method.
	(flow_sensitive_info_storage::restore): New method.
	(flow_sensitive_info_storage::save_and_clear): New method.
	(flow_sensitive_info_storage::clear_storage): New method.
	* tree-ssanames.h (class flow_sensitive_info_storage): New class.
---
 gcc/gimple-fold.cc   | 17 +++++------
 gcc/tree-ssanames.cc | 72 ++++++++++++++++++++++++++++++++++++++++++++
 gcc/tree-ssanames.h  | 21 +++++++++++++
 3 files changed, 100 insertions(+), 10 deletions(-)

diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
index 4027ff71e10..de94efbcff7 100644
--- a/gcc/gimple-fold.cc
+++ b/gcc/gimple-fold.cc
@@ -6947,7 +6947,7 @@ and_comparisons_1 (tree type, enum tree_code code1, tree op1a, tree op1b,
 }
 
 static basic_block fosa_bb;
-static vec<std::pair<tree, vrange_storage *> > *fosa_unwind;
+static vec<std::pair<tree, flow_sensitive_info_storage> > *fosa_unwind;
 static tree
 follow_outer_ssa_edges (tree val)
 {
@@ -6967,14 +6967,11 @@ follow_outer_ssa_edges (tree val)
 	   || POINTER_TYPE_P (TREE_TYPE (val)))
 	  && !TYPE_OVERFLOW_WRAPS (TREE_TYPE (val)))
 	return NULL_TREE;
+      flow_sensitive_info_storage storage;
+      storage.save_and_clear (val);
       /* If the definition does not dominate fosa_bb temporarily reset
 	 flow-sensitive info.  */
-      if (val->ssa_name.info.range_info)
-	{
-	  fosa_unwind->safe_push (std::make_pair
-				    (val, val->ssa_name.info.range_info));
-	  val->ssa_name.info.range_info = NULL;
-	}
+      fosa_unwind->safe_push (std::make_pair (val, storage));
       return val;
     }
   return val;
@@ -7034,14 +7031,14 @@ maybe_fold_comparisons_from_match_pd (tree type, enum tree_code code,
 		      type, gimple_assign_lhs (stmt1),
 		      gimple_assign_lhs (stmt2));
   fosa_bb = outer_cond_bb;
-  auto_vec<std::pair<tree, vrange_storage *>, 8> unwind_stack;
+  auto_vec<std::pair<tree, flow_sensitive_info_storage>, 8> unwind_stack;
   fosa_unwind = &unwind_stack;
   if (op.resimplify (NULL, (!outer_cond_bb
 			    ? follow_all_ssa_edges : follow_outer_ssa_edges)))
     {
       fosa_unwind = NULL;
       for (auto p : unwind_stack)
-	p.first->ssa_name.info.range_info = p.second;
+	p.second.restore (p.first);
       if (gimple_simplified_result_is_gimple_val (&op))
 	{
 	  tree res = op.ops[0];
@@ -7065,7 +7062,7 @@ maybe_fold_comparisons_from_match_pd (tree type, enum tree_code code,
     }
   fosa_unwind = NULL;
   for (auto p : unwind_stack)
-    p.first->ssa_name.info.range_info = p.second;
+    p.second.restore (p.first);
 
   return NULL_TREE;
 }
diff --git a/gcc/tree-ssanames.cc b/gcc/tree-ssanames.cc
index 5fdb6a37e9f..f81332451fc 100644
--- a/gcc/tree-ssanames.cc
+++ b/gcc/tree-ssanames.cc
@@ -916,3 +916,75 @@ make_pass_release_ssa_names (gcc::context *ctxt)
 {
   return new pass_release_ssa_names (ctxt);
 }
+
+/* Save and restore of flow sensitive information. */
+
+/* Save off the flow sensitive info from NAME. */
+
+void
+flow_sensitive_info_storage::save (tree name)
+{
+  gcc_assert (state == 0);
+  if (!POINTER_TYPE_P (TREE_TYPE (name)))
+    {
+      range_info = SSA_NAME_RANGE_INFO (name);
+      state = 1;
+      return;
+    }
+  state = -1;
+  auto ptr_info = SSA_NAME_PTR_INFO (name);
+  if (ptr_info)
+    {
+      align = ptr_info->align;
+      misalign = ptr_info->misalign;
+      null = SSA_NAME_PTR_INFO (name)->pt.null;
+    }
+  else
+    {
+      align = 0;
+      misalign = 0;
+      null = true;
+    }
+}
+
+/* Restore the flow sensitive info from NAME. */
+
+void
+flow_sensitive_info_storage::restore (tree name)
+{
+  gcc_assert (state != 0);
+  if (!POINTER_TYPE_P (TREE_TYPE (name)))
+    {
+      gcc_assert (state == 1);
+      SSA_NAME_RANGE_INFO (name) = range_info;
+      return;
+    }
+  gcc_assert (state == -1);
+  auto ptr_info = SSA_NAME_PTR_INFO (name);
+  /* If there was no flow sensitive info on the pointer
+     just return, there is nothing to restore to.  */
+  if (!ptr_info)
+    return;
+  if (align != 0)
+    set_ptr_info_alignment (ptr_info, align, misalign);
+  else
+    mark_ptr_info_alignment_unknown (ptr_info);
+  SSA_NAME_PTR_INFO (name)->pt.null = null;
+}
+
+/* Save off the flow sensitive info from NAME.
+   And reset the flow sensitive info of NAME. */
+
+void
+flow_sensitive_info_storage::save_and_clear (tree name)
+{
+  save (name);
+  reset_flow_sensitive_info (name);
+}
+
+/* Clear the storage. */
+void
+flow_sensitive_info_storage::clear_storage (void)
+{
+  state = 0;
+}
diff --git a/gcc/tree-ssanames.h b/gcc/tree-ssanames.h
index f3fa609208a..98f4a835483 100644
--- a/gcc/tree-ssanames.h
+++ b/gcc/tree-ssanames.h
@@ -136,5 +136,26 @@ make_temp_ssa_name (tree type, gimple *stmt, const char *name)
   return ssa_name;
 }
 
+/* A class which is used to save/restore the flow sensitive information.  */
+class flow_sensitive_info_storage
+{
+public:
+  void save (tree);
+  void save_and_clear (tree);
+  void restore (tree);
+  void clear_storage ();
+private:
+  /* 0 means there is nothing saved.
+     1 means non pointer is saved.
+     -1 means a pointer type is saved.
+     -2 means a pointer type is saved but no information was saved. */
+  int state = 0;
+  /* The range info for non pointers */
+  vrange_storage *range_info = nullptr;
+  /* Flow sensitive pointer information. */
+  unsigned int align = 0;
+  unsigned int misalign = 0;
+  bool null = true;
+};
 
 #endif /* GCC_TREE_SSANAMES_H */
-- 
2.31.1

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

[Andrew Pinski]

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.

This is similar to the previous version of the patch except now we use
flow_sensitive_info_storage instead of manually doing the save/restore
and also handle all defs on a gimple statement rather than just for lhs
of the gimple statement. Oh and a few more testcases were added that
was failing before.

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

	PR tree-optimization/110252

gcc/ChangeLog:

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

gcc/testsuite/ChangeLog:

	* gcc.dg/tree-ssa/phi-opt-25b.c: Updated as
	__builtin_parity loses the nonzerobits info.
	* gcc.c-torture/execute/pr110252-1.c: New test.
	* gcc.c-torture/execute/pr110252-2.c: New test.
	* gcc.c-torture/execute/pr110252-3.c: New test.
	* gcc.c-torture/execute/pr110252-4.c: New test.
---
 .../gcc.c-torture/execute/pr110252-1.c        | 15 ++++++
 .../gcc.c-torture/execute/pr110252-2.c        | 10 ++++
 .../gcc.c-torture/execute/pr110252-3.c        | 13 +++++
 .../gcc.c-torture/execute/pr110252-4.c        |  8 +++
 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-25b.c   |  6 +--
 gcc/tree-ssa-phiopt.cc                        | 51 +++++++++++++++++--
 6 files changed, 96 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-2.c
 create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr110252-3.c
 create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr110252-4.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-2.c b/gcc/testsuite/gcc.c-torture/execute/pr110252-2.c
new file mode 100644
index 00000000000..7f1a7dbf134
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr110252-2.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.c-torture/execute/pr110252-3.c b/gcc/testsuite/gcc.c-torture/execute/pr110252-3.c
new file mode 100644
index 00000000000..c24bf1ab1e4
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr110252-3.c
@@ -0,0 +1,13 @@
+
+unsigned int a = 1387579096U;
+void sinkandcheck(unsigned b) __attribute__((noipa));
+void sinkandcheck(unsigned b)
+{
+        if (a != b)
+        __builtin_abort();
+}
+int main() {
+    a = 1 < (~a) ? 1 : (~a);
+    sinkandcheck(1);
+    return 0;
+}
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr110252-4.c b/gcc/testsuite/gcc.c-torture/execute/pr110252-4.c
new file mode 100644
index 00000000000..f97edd3f069
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr110252-4.c
@@ -0,0 +1,8 @@
+
+int a, b = 2, c = 2;
+int main() {
+  b = ~(1 % (a ^ (b - (1 && c) || c & b)));
+  if (b < -1)
+    __builtin_abort();
+  return 0;
+}
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 467c9fd108a..9d542fd345f 100644
--- a/gcc/tree-ssa-phiopt.cc
+++ b/gcc/tree-ssa-phiopt.cc
@@ -708,6 +708,45 @@ move_stmt (gimple *stmt, gimple_stmt_iterator *gsi, auto_bitmap &inserted_exprs)
   reset_flow_sensitive_info (name);
 }
 
+/* RAII style class to temporarily remove flow sensitive
+   from ssa names defined by a gimple statement.  */
+class auto_flow_sensitive
+{
+public:
+  auto_flow_sensitive (gimple *s);
+  ~auto_flow_sensitive ();
+private:
+  auto_vec<std::pair<tree, flow_sensitive_info_storage>, 2> stack;
+};
+
+/* Constructor for auto_flow_sensitive. Saves
+   off the ssa names' 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;
+  ssa_op_iter it;
+  tree def;
+  FOR_EACH_SSA_TREE_OPERAND (def, s, it, SSA_OP_DEF)
+    {
+      flow_sensitive_info_storage storage;
+      storage.save_and_clear (def);
+      stack.safe_push (std::make_pair (def, storage));
+    }
+}
+
+/* Deconstructor, restores the flow sensitive information
+   for the SSA names that had been saved off.  */
+
+auto_flow_sensitive::~auto_flow_sensitive ()
+{
+  for (auto p : stack)
+    p.second.restore (p.first);
+}
+
 /*  The function match_simplify_replacement does the main work of doing the
     replacement using match and simplify.  Return true if the replacement is done.
     Otherwise return false.
@@ -793,9 +832,15 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
     return false;
 
   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;
 
-- 
2.31.1

Re: [PATCH 1/2] Add flow_sensitive_info_storage and use it in gimple-fold.

[Richard Biener]

On Sat, Jul 15, 2023 at 5:21 AM Andrew Pinski via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> This adds flow_sensitive_info_storage and uses it in
> maybe_fold_comparisons_from_match_pd as mentioned in
> https://gcc.gnu.org/pipermail/gcc-patches/2023-June/621817.html .
> Since using it in maybe_fold_comparisons_from_match_pd was easy
> and allowed me to test the storage earlier, I did it.
>
> This also hides better how the flow sensitive information is
> stored and only a single place needs to be updated if that
> ever changes (again).
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

OK.

Thanks for doing this!
Richard.

> gcc/ChangeLog:
>
>         * gimple-fold.cc (fosa_unwind): Replace `vrange_storage *`
>         with flow_sensitive_info_storage.
>         (follow_outer_ssa_edges): Update how to save off the flow
>         sensitive info.
>         (maybe_fold_comparisons_from_match_pd): Update restoring
>         of flow sensitive info.
>         * tree-ssanames.cc (flow_sensitive_info_storage::save): New method.
>         (flow_sensitive_info_storage::restore): New method.
>         (flow_sensitive_info_storage::save_and_clear): New method.
>         (flow_sensitive_info_storage::clear_storage): New method.
>         * tree-ssanames.h (class flow_sensitive_info_storage): New class.
> ---
>  gcc/gimple-fold.cc   | 17 +++++------
>  gcc/tree-ssanames.cc | 72 ++++++++++++++++++++++++++++++++++++++++++++
>  gcc/tree-ssanames.h  | 21 +++++++++++++
>  3 files changed, 100 insertions(+), 10 deletions(-)
>
> diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
> index 4027ff71e10..de94efbcff7 100644
> --- a/gcc/gimple-fold.cc
> +++ b/gcc/gimple-fold.cc
> @@ -6947,7 +6947,7 @@ and_comparisons_1 (tree type, enum tree_code code1, tree op1a, tree op1b,
>  }
>
>  static basic_block fosa_bb;
> -static vec<std::pair<tree, vrange_storage *> > *fosa_unwind;
> +static vec<std::pair<tree, flow_sensitive_info_storage> > *fosa_unwind;
>  static tree
>  follow_outer_ssa_edges (tree val)
>  {
> @@ -6967,14 +6967,11 @@ follow_outer_ssa_edges (tree val)
>            || POINTER_TYPE_P (TREE_TYPE (val)))
>           && !TYPE_OVERFLOW_WRAPS (TREE_TYPE (val)))
>         return NULL_TREE;
> +      flow_sensitive_info_storage storage;
> +      storage.save_and_clear (val);
>        /* If the definition does not dominate fosa_bb temporarily reset
>          flow-sensitive info.  */
> -      if (val->ssa_name.info.range_info)
> -       {
> -         fosa_unwind->safe_push (std::make_pair
> -                                   (val, val->ssa_name.info.range_info));
> -         val->ssa_name.info.range_info = NULL;
> -       }
> +      fosa_unwind->safe_push (std::make_pair (val, storage));
>        return val;
>      }
>    return val;
> @@ -7034,14 +7031,14 @@ maybe_fold_comparisons_from_match_pd (tree type, enum tree_code code,
>                       type, gimple_assign_lhs (stmt1),
>                       gimple_assign_lhs (stmt2));
>    fosa_bb = outer_cond_bb;
> -  auto_vec<std::pair<tree, vrange_storage *>, 8> unwind_stack;
> +  auto_vec<std::pair<tree, flow_sensitive_info_storage>, 8> unwind_stack;
>    fosa_unwind = &unwind_stack;
>    if (op.resimplify (NULL, (!outer_cond_bb
>                             ? follow_all_ssa_edges : follow_outer_ssa_edges)))
>      {
>        fosa_unwind = NULL;
>        for (auto p : unwind_stack)
> -       p.first->ssa_name.info.range_info = p.second;
> +       p.second.restore (p.first);
>        if (gimple_simplified_result_is_gimple_val (&op))
>         {
>           tree res = op.ops[0];
> @@ -7065,7 +7062,7 @@ maybe_fold_comparisons_from_match_pd (tree type, enum tree_code code,
>      }
>    fosa_unwind = NULL;
>    for (auto p : unwind_stack)
> -    p.first->ssa_name.info.range_info = p.second;
> +    p.second.restore (p.first);
>
>    return NULL_TREE;
>  }
> diff --git a/gcc/tree-ssanames.cc b/gcc/tree-ssanames.cc
> index 5fdb6a37e9f..f81332451fc 100644
> --- a/gcc/tree-ssanames.cc
> +++ b/gcc/tree-ssanames.cc
> @@ -916,3 +916,75 @@ make_pass_release_ssa_names (gcc::context *ctxt)
>  {
>    return new pass_release_ssa_names (ctxt);
>  }
> +
> +/* Save and restore of flow sensitive information. */
> +
> +/* Save off the flow sensitive info from NAME. */
> +
> +void
> +flow_sensitive_info_storage::save (tree name)
> +{
> +  gcc_assert (state == 0);
> +  if (!POINTER_TYPE_P (TREE_TYPE (name)))
> +    {
> +      range_info = SSA_NAME_RANGE_INFO (name);
> +      state = 1;
> +      return;
> +    }
> +  state = -1;
> +  auto ptr_info = SSA_NAME_PTR_INFO (name);
> +  if (ptr_info)
> +    {
> +      align = ptr_info->align;
> +      misalign = ptr_info->misalign;
> +      null = SSA_NAME_PTR_INFO (name)->pt.null;
> +    }
> +  else
> +    {
> +      align = 0;
> +      misalign = 0;
> +      null = true;
> +    }
> +}
> +
> +/* Restore the flow sensitive info from NAME. */
> +
> +void
> +flow_sensitive_info_storage::restore (tree name)
> +{
> +  gcc_assert (state != 0);
> +  if (!POINTER_TYPE_P (TREE_TYPE (name)))
> +    {
> +      gcc_assert (state == 1);
> +      SSA_NAME_RANGE_INFO (name) = range_info;
> +      return;
> +    }
> +  gcc_assert (state == -1);
> +  auto ptr_info = SSA_NAME_PTR_INFO (name);
> +  /* If there was no flow sensitive info on the pointer
> +     just return, there is nothing to restore to.  */
> +  if (!ptr_info)
> +    return;
> +  if (align != 0)
> +    set_ptr_info_alignment (ptr_info, align, misalign);
> +  else
> +    mark_ptr_info_alignment_unknown (ptr_info);
> +  SSA_NAME_PTR_INFO (name)->pt.null = null;
> +}
> +
> +/* Save off the flow sensitive info from NAME.
> +   And reset the flow sensitive info of NAME. */
> +
> +void
> +flow_sensitive_info_storage::save_and_clear (tree name)
> +{
> +  save (name);
> +  reset_flow_sensitive_info (name);
> +}
> +
> +/* Clear the storage. */
> +void
> +flow_sensitive_info_storage::clear_storage (void)
> +{
> +  state = 0;
> +}
> diff --git a/gcc/tree-ssanames.h b/gcc/tree-ssanames.h
> index f3fa609208a..98f4a835483 100644
> --- a/gcc/tree-ssanames.h
> +++ b/gcc/tree-ssanames.h
> @@ -136,5 +136,26 @@ make_temp_ssa_name (tree type, gimple *stmt, const char *name)
>    return ssa_name;
>  }
>
> +/* A class which is used to save/restore the flow sensitive information.  */
> +class flow_sensitive_info_storage
> +{
> +public:
> +  void save (tree);
> +  void save_and_clear (tree);
> +  void restore (tree);
> +  void clear_storage ();
> +private:
> +  /* 0 means there is nothing saved.
> +     1 means non pointer is saved.
> +     -1 means a pointer type is saved.
> +     -2 means a pointer type is saved but no information was saved. */
> +  int state = 0;
> +  /* The range info for non pointers */
> +  vrange_storage *range_info = nullptr;
> +  /* Flow sensitive pointer information. */
> +  unsigned int align = 0;
> +  unsigned int misalign = 0;
> +  bool null = true;
> +};
>
>  #endif /* GCC_TREE_SSANAMES_H */
> --
> 2.31.1
>

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

[Richard Biener]

On Sat, Jul 15, 2023 at 5:21 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.
>
> This is similar to the previous version of the patch except now we use
> flow_sensitive_info_storage instead of manually doing the save/restore
> and also handle all defs on a gimple statement rather than just for lhs
> of the gimple statement. Oh and a few more testcases were added that
> was failing before.
>
> OK? Bootsrapped and tested on x86_64-linux-gnu with no regressions.

OK.

Thanks,
Richard.

>         PR tree-optimization/110252
>
> gcc/ChangeLog:
>
>         * tree-ssa-phiopt.cc (class auto_flow_sensitive): New class.
>         (auto_flow_sensitive::auto_flow_sensitive): New constructor.
>         (auto_flow_sensitive::~auto_flow_sensitive): New deconstructor.
>         (match_simplify_replacement): Temporarily
>         remove the flow sensitive info on the two statements that might
>         be moved.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.dg/tree-ssa/phi-opt-25b.c: Updated as
>         __builtin_parity loses the nonzerobits info.
>         * gcc.c-torture/execute/pr110252-1.c: New test.
>         * gcc.c-torture/execute/pr110252-2.c: New test.
>         * gcc.c-torture/execute/pr110252-3.c: New test.
>         * gcc.c-torture/execute/pr110252-4.c: New test.
> ---
>  .../gcc.c-torture/execute/pr110252-1.c        | 15 ++++++
>  .../gcc.c-torture/execute/pr110252-2.c        | 10 ++++
>  .../gcc.c-torture/execute/pr110252-3.c        | 13 +++++
>  .../gcc.c-torture/execute/pr110252-4.c        |  8 +++
>  gcc/testsuite/gcc.dg/tree-ssa/phi-opt-25b.c   |  6 +--
>  gcc/tree-ssa-phiopt.cc                        | 51 +++++++++++++++++--
>  6 files changed, 96 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-2.c
>  create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr110252-3.c
>  create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr110252-4.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-2.c b/gcc/testsuite/gcc.c-torture/execute/pr110252-2.c
> new file mode 100644
> index 00000000000..7f1a7dbf134
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr110252-2.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.c-torture/execute/pr110252-3.c b/gcc/testsuite/gcc.c-torture/execute/pr110252-3.c
> new file mode 100644
> index 00000000000..c24bf1ab1e4
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr110252-3.c
> @@ -0,0 +1,13 @@
> +
> +unsigned int a = 1387579096U;
> +void sinkandcheck(unsigned b) __attribute__((noipa));
> +void sinkandcheck(unsigned b)
> +{
> +        if (a != b)
> +        __builtin_abort();
> +}
> +int main() {
> +    a = 1 < (~a) ? 1 : (~a);
> +    sinkandcheck(1);
> +    return 0;
> +}
> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr110252-4.c b/gcc/testsuite/gcc.c-torture/execute/pr110252-4.c
> new file mode 100644
> index 00000000000..f97edd3f069
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr110252-4.c
> @@ -0,0 +1,8 @@
> +
> +int a, b = 2, c = 2;
> +int main() {
> +  b = ~(1 % (a ^ (b - (1 && c) || c & b)));
> +  if (b < -1)
> +    __builtin_abort();
> +  return 0;
> +}
> 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 467c9fd108a..9d542fd345f 100644
> --- a/gcc/tree-ssa-phiopt.cc
> +++ b/gcc/tree-ssa-phiopt.cc
> @@ -708,6 +708,45 @@ move_stmt (gimple *stmt, gimple_stmt_iterator *gsi, auto_bitmap &inserted_exprs)
>    reset_flow_sensitive_info (name);
>  }
>
> +/* RAII style class to temporarily remove flow sensitive
> +   from ssa names defined by a gimple statement.  */
> +class auto_flow_sensitive
> +{
> +public:
> +  auto_flow_sensitive (gimple *s);
> +  ~auto_flow_sensitive ();
> +private:
> +  auto_vec<std::pair<tree, flow_sensitive_info_storage>, 2> stack;
> +};
> +
> +/* Constructor for auto_flow_sensitive. Saves
> +   off the ssa names' 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;
> +  ssa_op_iter it;
> +  tree def;
> +  FOR_EACH_SSA_TREE_OPERAND (def, s, it, SSA_OP_DEF)
> +    {
> +      flow_sensitive_info_storage storage;
> +      storage.save_and_clear (def);
> +      stack.safe_push (std::make_pair (def, storage));
> +    }
> +}
> +
> +/* Deconstructor, restores the flow sensitive information
> +   for the SSA names that had been saved off.  */
> +
> +auto_flow_sensitive::~auto_flow_sensitive ()
> +{
> +  for (auto p : stack)
> +    p.second.restore (p.first);
> +}
> +
>  /*  The function match_simplify_replacement does the main work of doing the
>      replacement using match and simplify.  Return true if the replacement is done.
>      Otherwise return false.
> @@ -793,9 +832,15 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
>      return false;
>
>    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;
>
> --
> 2.31.1
>