On Tue, Jun 9, 2026 at 1:28 AM Richard Biener
<[email protected]> wrote:
>
> On Mon, Jun 8, 2026 at 9:46 PM Andrew Pinski
> <[email protected]> wrote:
> >
> > On Mon, Jun 8, 2026 at 2:47 AM Richard Biener
> > <[email protected]> wrote:
> > >
> > > On Fri, Jun 5, 2026 at 7:43 AM Andrew Pinski
> > > <[email protected]> wrote:
> > > >
> > > > r17-1273-g391ee229b737eb added support for the case where the middle bb
> > > > was non-empty but
> > > > with a trailing store. This meant if there was a load in the middle bb,
> > > > it might cause
> > > > nontrapping to have the lhs in it. So we now need to check for a load
> > > > in the middle-bb
> > > > to reject this case.
> > > >
> > > > Bootstrapped and tested on x86_64-linux-gnu.
> > > >
> > > > PR tree-optimization/125612
> > > >
> > > > gcc/ChangeLog:
> > > >
> > > > * tree-ssa-phiopt.cc (cond_store_replacement): For the case
> > > > where
> > > > lhs is "known" to be nontrapping make sure there are no loads in
> > > > the middle bb.
> > > >
> > > > gcc/testsuite/ChangeLog:
> > > >
> > > > * gcc.dg/tree-ssa/pr125612-1.c: New test.
> > > >
> > > > Signed-off-by: Andrew Pinski <[email protected]>
> > > > ---
> > > > gcc/testsuite/gcc.dg/tree-ssa/pr125612-1.c | 26 ++++++++++++++++++++++
> > > > gcc/tree-ssa-phiopt.cc | 19 +++++++++++++++-
> > > > 2 files changed, 44 insertions(+), 1 deletion(-)
> > > > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr125612-1.c
> > > >
> > > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr125612-1.c
> > > > b/gcc/testsuite/gcc.dg/tree-ssa/pr125612-1.c
> > > > new file mode 100644
> > > > index 00000000000..201f2f4fb3f
> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr125612-1.c
> > > > @@ -0,0 +1,26 @@
> > > > +/* { dg-do run } */
> > > > +/* { dg-options "-O3 -fno-tree-dse -fno-tree-dce
> > > > -fdump-tree-cselim-details" } */
> > > > +/* PR tree-optimization/125612 */
> > > > +
> > > > +/* The conditional store of `h[2000]` should not become unconditional
> > > > since
> > > > + it traps. */
> > > > +
> > > > +int a, b, c = 2, d, e[4], f, i;
> > > > +void g() {
> > > > + int h[5];
> > > > + e[0] = 2;
> > > > + while (1) {
> > > > + if (e[b]) {
> > > > + if (!c)
> > > > + h[2000] &= 1;
> > > > + return;
> > > > + }
> > > > + }
> > > > +}
> > > > +int main() {
> > > > + for (; i < 4; i++)
> > > > + g();
> > > > + return 0;
> > > > +}
> > > > +
> > > > +/* { dg-final { scan-tree-dump-not "Conditional store replacement
> > > > happened" "cselim"} } */
> > > > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> > > > index 3e7dd3b0ee3..0c5c4982d3f 100644
> > > > --- a/gcc/tree-ssa-phiopt.cc
> > > > +++ b/gcc/tree-ssa-phiopt.cc
> > > > @@ -3089,7 +3089,24 @@ cond_store_replacement (basic_block middle_bb,
> > > > basic_block join_bb, edge e0,
> > > > /* Prove that we can move the store down. We could also check
> > > > TREE_THIS_NOTRAP here, but in that case we also could move stores,
> > > > whose value is not available readily, which we want to avoid. */
> > > > - if (!nontrap->contains (lhs))
> > > > + if (nontrap->contains (lhs))
> > > > + {
> > > > + /* Make sure there is no load in the middle bb,
> > > > + this invalidates nontrap.
> > >
> > > How so? nontrap should only contain the load, not the store? We mark
> > > a MEM as not trapping if there's a dominating same MEM only and
> > > nontrap contains the actual tree (which is always unshared)
> >
> > Right, there is a load from the same lhs that dominates the same MEM but
> > only
> > in that branch. e.g.
> > ```
> > if (a)
> > {
> > int t = d[1024];
> > t = t | 1;
> > d[1024] = t;
> > }
> > ```
> > Here the load is dominating the store. I could have checked to see if
> > the load was the same but I didn't think it was that useful. Since
> > this just disables the transformation to what was done before
> > Pengxuan's patch in the case of the load there; I thought this was the
> > safest thing to do.
>
> Ah, I suppose it would be useful to record the dominating BB that
> specifies the region the MEM is non-trapping in (so change the hash_set
> to a hash_map). In this case it would be the BB of the store itself, and
> moving it out of the if() moves it out of that dominating region.
I am not sure if the load case in the middle bb would show up that
often for the new cs-elim. So changing how this is done might not be
useful.
Looking for the load before the store in the middle bb, after the fact
is simple and still cheap. As I mentioned we could check if the load
is the same as the store but I didn't see a need for that check.
>
> Your comment isn't really clear IMO - it's that nontrap info only is valid
> for the current position of the store, just by extension with no load we
> ruled out the load must have been elsewhere.
Right, I will change it to be better soon. As I agree, the comment is
definitely misleading and incorrect.
> Btw, same issue with
>
> if (a)
> {
> d[1024] = x;
> t = t | 1;
> d[1024] = t;
> }
>
> IIRC we now allow more than one store in the middle block as well?
We don't, as cselim_candidate calls trailing_store_in_bb with true as
the last argument which checks to see if it is the only store in the
middle block.
Thanks,
Andrea
>
> >
> > Thanks,
> > Andrea
> >
> > >
> > > > + FIXME: this is over conserative, this check could be made to
> > > > + allow loads unrelated to lhs. */
> > > > + tree vuse = gimple_vuse (assign);
> > > > + imm_use_iterator iter;
> > > > + gimple *use_stmt;
> > > > + FOR_EACH_IMM_USE_STMT (use_stmt, iter, vuse)
> > > > + {
> > > > + if (use_stmt == assign)
> > > > + continue;
> > > > + if (gimple_bb (use_stmt) == middle_bb)
> > > > + return false;
> > > > + }
> > > > + }
> > > > + else
> > > > {
> > > > /* If LHS is an access to a local variable without address-taken
> > > > (or when we allow data races) and known not to trap, we could
> > > > --
> > > > 2.43.0
> > > >
