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.
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. 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?
>
> 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
> > >
