I noticed this while working on std::optional, the full blown cselim with data
references
hanldes the case where there are a few stores on either side of the branch that
don't
interfer with the other stores. We could just support clobbers but that would
have
a similar code except for an extra check for clobbers. So let's just add
support
for skipping on store on either side of the conditional too.
The change to trailing_store_in_bb simplifies some things as already the other
side would check on the store being a simple assign, this does not change
anything.
Note cselim-1.c was reduced from a draft version of this patch which was
miscompiling isl.
In the whole testsuite the only regressions were in graphite. So I thought it
would
be a good idea to have a testcase to be added for the miscompiling case. I had
forgot
some stores could also be loads so another check was needed in that case.
This was found by looking into std::optional::reset code gen.
Changes since v1:
* v2: skip stores and not just clobbers
Bootstrapped and tested on x86_64-linux-gnu.
PR tree-optimization/127053
gcc/ChangeLog:
* tree-ssa-phiopt.cc (trailing_store_in_bb): Rename vphi to
vuse_only.
(cond_if_else_store_replacement_limited): Add case if
cond_if_else_store_replacement_1 failed, allowing
for skipping one store on each side.
(trailing_store_in_bb): Require the store stmt
to be a gimple simple assign.
gcc/testsuite/ChangeLog:
* g++.dg/tree-ssa/cselim-2.C: New test.
* gcc.dg/torture/cselim-1.c: New test.
Signed-off-by: Andrea Pinski <[email protected]>
---
gcc/testsuite/g++.dg/tree-ssa/cselim-2.C | 23 ++++++++++
gcc/testsuite/gcc.dg/torture/cselim-1.c | 34 ++++++++++++++
gcc/tree-ssa-phiopt.cc | 56 +++++++++++++++++++++---
3 files changed, 107 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/tree-ssa/cselim-2.C
create mode 100644 gcc/testsuite/gcc.dg/torture/cselim-1.c
diff --git a/gcc/testsuite/g++.dg/tree-ssa/cselim-2.C
b/gcc/testsuite/g++.dg/tree-ssa/cselim-2.C
new file mode 100644
index 00000000000..9c040f9d183
--- /dev/null
+++ b/gcc/testsuite/g++.dg/tree-ssa/cselim-2.C
@@ -0,0 +1,23 @@
+// PR tree-optimization/127053
+// { dg-do compile }
+// { dg-options "-O2 -ftree-cselim -fdump-tree-phiopt1-details" }
+// testcase reduced from std::optional::reset.
+
+void sink(int*);
+typedef int T;
+
+int f(int b, T *c)
+{
+ int a;
+ sink(&a);
+ if (b)
+ {
+ a = 0;
+ c->~T();
+ }
+ else
+ a = 0;
+ return a;
+}
+
+// { dg-final { scan-tree-dump "factoring out stores" "phiopt1" } }
diff --git a/gcc/testsuite/gcc.dg/torture/cselim-1.c
b/gcc/testsuite/gcc.dg/torture/cselim-1.c
new file mode 100644
index 00000000000..4133f627b26
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/cselim-1.c
@@ -0,0 +1,34 @@
+/* { dg-do run } */
+
+/* CS-ELIM was miscompiling this
+ by moving the store of v1.t across
+ the load of v1 in `v2 = v1`. */
+
+/* This was reduced from isl manually. */
+
+struct s1 {
+ int t;
+};
+
+struct s1 v1, v2;
+
+[[gnu::noipa]]
+void f(int a) {
+ if (a) {
+ v1.t = 1;
+ v2 = v1;
+ } else {
+ v1.t = 2;
+ v2 = v1;
+ }
+}
+
+int main()
+{
+ f(1);
+ if (v2.t != 1)
+ __builtin_abort ();
+ f(0);
+ if (v2.t != 2)
+ __builtin_abort ();
+}
diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
index 343cd159118..c1a32106a00 100644
--- a/gcc/tree-ssa-phiopt.cc
+++ b/gcc/tree-ssa-phiopt.cc
@@ -3503,12 +3503,12 @@ cond_if_else_store_replacement_1 (basic_block then_bb,
basic_block else_bb,
}
/* Return the last store in BB with VDEF or NULL if there are
- loads following the store. VPHI is where the only use of the
+ loads following the store. VUSE_ONLY is where the only use of the
vdef should be. If ONLYONESTORE is true, then the store is
the only store in the BB. */
static gimple *
-trailing_store_in_bb (basic_block bb, tree vdef, gphi *vphi, bool onlyonestore)
+trailing_store_in_bb (basic_block bb, tree vdef, gimple *vuse_only, bool
onlyonestore)
{
if (SSA_NAME_IS_DEFAULT_DEF (vdef))
return NULL;
@@ -3524,14 +3524,17 @@ trailing_store_in_bb (basic_block bb, tree vdef, gphi
*vphi, bool onlyonestore)
&& gimple_code (SSA_NAME_DEF_STMT (gimple_vuse (store))) != GIMPLE_PHI)
return NULL;
+ // Needs to be an simple store and not a call.
+ if (!gimple_assign_single_p (store))
+ return NULL;
/* Verify there is no load or store after the store, the vdef of the store
- should only be used by the vphi joining the 2 bbs. */
+ should only be used by the vuse_only. */
use_operand_p use_p;
gimple *use_stmt;
if (!single_imm_use (gimple_vdef (store), &use_p, &use_stmt))
return NULL;
- if (use_stmt != vphi)
+ if (use_stmt != vuse_only)
return NULL;
return store;
@@ -3854,8 +3857,49 @@ cond_if_else_store_replacement_limited (basic_block
then_bb, basic_block else_bb
if (!else_assign)
return false;
- return cond_if_else_store_replacement_1 (then_bb, else_bb, join_bb,
- then_assign, else_assign, vphi);
+ if (!cond_if_else_store_replacement_1 (then_bb, else_bb, join_bb,
+ then_assign, else_assign, vphi))
+ {
+ gimple *then_n, *else_n;
+
+ // Try to see if one "store" can be skipped on either side.
+ if (!flag_expensive_optimizations)
+ return false;
+ then_n = trailing_store_in_bb (then_bb, gimple_vuse (then_assign),
+ then_assign, true);
+ if (then_n)
+ {
+ ao_ref then_ref_n;
+ ao_ref_init (&then_ref_n, gimple_assign_lhs (then_n));
+ if (stmt_may_clobber_ref_p_1 (then_assign, &then_ref_n, false)
+ || ref_maybe_used_by_stmt_p (then_assign, &then_ref_n, false))
+ then_n = nullptr;
+ }
+ else_n = trailing_store_in_bb (else_bb, gimple_vuse (else_assign),
+ else_assign, true);
+ if (else_n)
+ {
+ ao_ref else_ref_n;
+ ao_ref_init (&else_ref_n, gimple_assign_lhs (else_n));
+ if (stmt_may_clobber_ref_p_1 (else_assign, &else_ref_n, false)
+ || ref_maybe_used_by_stmt_p (else_assign, &else_ref_n, false))
+ else_n = nullptr;
+ }
+ if (then_n
+ && cond_if_else_store_replacement_1 (then_bb, else_bb, join_bb,
+ then_n, else_assign, vphi))
+ return true;
+ if (else_n
+ && cond_if_else_store_replacement_1 (then_bb, else_bb, join_bb,
+ then_assign, else_n, vphi))
+ return true;
+ if (else_n && then_n
+ && cond_if_else_store_replacement_1 (then_bb, else_bb, join_bb,
+ then_n, else_n, vphi))
+ return true;
+ return false;
+ }
+ return true;
}
/* Conditional store replacement. We already know
--
2.43.0
