[2/2] Fix tree-opt/110252: wrong code due to phiopt using flow sensitive info during match
Checks
Commit Message
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
Comments
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
>
new file mode 100644
@@ -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();
+ }
+}
new file mode 100644
@@ -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();
+}
new file mode 100644
@@ -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;
+}
new file mode 100644
@@ -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;
+}
@@ -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" } } */
@@ -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;