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
next 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).