public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Andrew Pinski <apinski@marvell.com>
To: <gcc-patches@gcc.gnu.org>
Cc: Andrew Pinski <apinski@marvell.com>
Subject: [PATCH] Fix tree-opt/110252: wrong code due to phiopt using flow sensitive info during match
Date: Wed, 14 Jun 2023 23:17:04 -0700	[thread overview]
Message-ID: <20230615061704.3514834-1-apinski@marvell.com> (raw)

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

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


             reply	other threads:[~2023-06-15  6:17 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-06-15  6:17 Andrew Pinski [this message]
2023-06-15  6:58 ` Richard Biener

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230615061704.3514834-1-apinski@marvell.com \
    --to=apinski@marvell.com \
    --cc=gcc-patches@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).