https://gcc.gnu.org/g:e35b8dad030bc4164fdaa81dc0a5bfca8c1bbfc3
commit r17-4469-ge35b8dad030bc4164fdaa81dc0a5bfca8c1bbfc3 Author: Andrea Pinski <[email protected]> Date: Tue Aug 25 14:47:38 2026 -0700 cselim: cond_if_else_store_replacement_limited support skipping on store one either side [PR127053] 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]> Diff: --- 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(-) 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 000000000000..9c040f9d1839 --- /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 000000000000..4133f627b26f --- /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 84a7949583d6..7e13c8ff129f 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
