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

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

[Andrew Pinski]

https://gcc.gnu.org/g:8c79b49cd4fa742f7be739dd21fd2aa040cc1ba3

commit r14-2650-g8c79b49cd4fa742f7be739dd21fd2aa040cc1ba3
Author: Andrew Pinski <apinski@marvell.com>
Date:   Fri Jul 14 15:55:34 2023 -0700

    [PATCH] 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.
    
    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.

Diff:
---
 gcc/testsuite/gcc.c-torture/execute/pr110252-1.c | 15 +++++++
 gcc/testsuite/gcc.c-torture/execute/pr110252-2.c | 10 +++++
 gcc/testsuite/gcc.c-torture/execute/pr110252-3.c | 13 ++++++
 gcc/testsuite/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(-)

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;