On Tue, 25 Aug 2026, Andrea Pinski wrote:
> On Tue, Aug 25, 2026 at 6:06 AM <[email protected]> wrote:
> >
> > From: Kyrylo Tkachov <[email protected]>
> >
> > The fix for PR120944 stopped value-numbering from using the value of any
> > store whose left-hand side is a volatile lvalue. That is stronger than
> > needed: what the PR needs is that a volatile *load* accesses memory again.
> > A store determines the contents of the object it writes whether or not the
> > lvalue it is performed through is volatile-qualified, so a later
> > non-volatile load of the same location may take the stored value: either
> > the object itself is not volatile-qualified, in which case nothing else can
> > change it, or it is and reading it through a non-volatile lvalue is
> > undefined (C11 6.7.3p6).
> >
> > Record in the walk data whether the reference being looked up is a volatile
> > access, and when it is refuse every store rather than only a volatile one.
> > A volatile load has to reach memory, so no stored value may serve it,
> > whatever lvalue the store was performed through. The looked-up reference
> > has its volatile flag cleared by ao_ref_init_from_vn_reference, so take the
> > flag from the original tree in vn_reference_lookup.
> >
> > The by-pieces entry point has no tree to inspect, so a second flag records
> > that the volatility could not be determined and keeps that path on the
> > narrower rule of refusing a store performed through a volatile lvalue,
> > which is what it did before. Both flags are needed because the two mean
> > different things: one says the load is volatile, the other says we do not
> > know. Collapsing them would make vn_reference_lookup_pieces refuse every
> > store, which would remove store-to-load forwarding from PRE's phi_translate
> > and compute_avail.
> >
> > unsigned f (unsigned *p, unsigned v)
> > {
> > *(volatile unsigned *) p = v;
> > return p[0];
> > }
> >
> > before: after:
> > str w1, [x0] str w1, [x0]
> > ldr w0, [x0] mov w0, w1
> > ret ret
> >
> > sha1dc, the SHA-1 implementation with collision detection that git and
> > libgit2 use by default, stores its 80-word message schedule through a
> > volatile lvalue and reads it back with plain loads, so every round reloaded
> > words it had just written.
>
> That seems like a broken code in the first place. Why are they using
> volatile here? I suspect they are using volatile to get around some
> aliasing violations. I think we should push the code to be fixed
> rather than change how volatile (stores and loads) are invisible to
> GCC optimizers.
I agree. This seems to be a very odd optimization, also a questionable
in my view. The volatile store basically says "this memory location
is volatile", so a read from it as non-volatile might even be considered
UB (sort-of a different TBAA set). Magically interpreting the storage
as no longer volatile might be OK, but then re-interpreting the store
as to non-volatile memory is IMO not.
Consider
volatile int i;
void foo ()
{
i = 1;
return *(int *)&i;
}
now the storage is even _declared_ volatile. Would you still think
your optimization is sound? IMO it's much more obvious questionable here.
Richard.
> > On a fixed-work SHA-1 hashing loop on Grace
> > the patch removes 2.3% of dynamic instructions and 8.4% of cycles, which
> > brings GCC to within 1% of clang 22 on the same source
> > (Clang does not emit the loads). The number of
> > loads in sha1_compression_states drops by about half. The volatile stores
> > are all still emitted and are not merged into pairs.
> >
> > Bootstrapped and tested on aarch64-none-linux-gnu and x86_64-linux.
> > Ok for trunk?
> > Thanks,
> > Kyrill
> >
> > gcc/ChangeLog:
> >
> > PR tree-optimization/120944
> > * tree-ssa-sccvn.cc (vn_walk_cb_data::vn_walk_cb_data): Add
> > ORIG_VOLATILE_P_ and UNKNOWN_VOLATILE_P_ parameters.
> > (vn_walk_cb_data::orig_volatile_p): New member.
> > (vn_walk_cb_data::unknown_volatile_p): New member.
> > (vn_stored_value_available_p): New function.
> > (vn_reference_lookup_3): Use it instead of testing the volatility
> > of the store left-hand side directly.
> > (vn_reference_lookup): Pass the volatility of OP.
> >
> > gcc/testsuite/ChangeLog:
> >
> > PR tree-optimization/120944
> > * gcc.dg/tree-ssa/ssa-fre-115.c: New test.
> >
> > Signed-off-by: Kyrylo Tkachov <[email protected]>
> > ---
> > gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-115.c | 23 +++++++++++
> > gcc/tree-ssa-sccvn.cc | 46 ++++++++++++++++++---
> > 2 files changed, 64 insertions(+), 5 deletions(-)
> > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-115.c
> >
> > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-115.c
> > b/gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-115.c
> > new file mode 100644
> > index 00000000000..7ccbc87f816
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-115.c
> > @@ -0,0 +1,23 @@
> > +/* Value-numbering may forward a value stored through a volatile lvalue
> > + to a later non-volatile load of the same location. The store itself
> > + has to stay. */
> > +
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -fdump-tree-fre1" } */
> > +
> > +unsigned int
> > +f (unsigned int *p, unsigned int v)
> > +{
> > + *(volatile unsigned int *) p = v;
> > + return p[0];
> > +}
> > +
> > +unsigned int
> > +g (unsigned int *p)
> > +{
> > + *(volatile unsigned int *) p = 42;
> > + return p[0];
> > +}
> > +
> > +/* { dg-final { scan-tree-dump-not "= \\*p_" "fre1" } } */
> > +/* { dg-final { scan-tree-dump-times "={v}" 2 "fre1" } } */
> > diff --git a/gcc/tree-ssa-sccvn.cc b/gcc/tree-ssa-sccvn.cc
> > index 840bcf70e0a..dc4bce149dc 100644
> > --- a/gcc/tree-ssa-sccvn.cc
> > +++ b/gcc/tree-ssa-sccvn.cc
> > @@ -1935,11 +1935,15 @@ struct vn_walk_cb_data
> > {
> > vn_walk_cb_data (vn_reference_t vr_, tree orig_ref_, tree
> > *last_vuse_ptr_,
> > vn_lookup_kind vn_walk_kind_, bool tbaa_p_, tree mask_,
> > - bool redundant_store_removal_p_)
> > + bool redundant_store_removal_p_,
> > + bool orig_volatile_p_ = false,
> > + bool unknown_volatile_p_ = true)
> > : vr (vr_), last_vuse_ptr (last_vuse_ptr_), last_vuse (NULL_TREE),
> > mask (mask_), masked_result (NULL_TREE), same_val (NULL_TREE),
> > vn_walk_kind (vn_walk_kind_),
> > tbaa_p (tbaa_p_), redundant_store_removal_p
> > (redundant_store_removal_p_),
> > + orig_volatile_p (orig_volatile_p_),
> > + unknown_volatile_p (unknown_volatile_p_),
> > saved_operands (vNULL), first_range (), first_set (-2),
> > first_base_set (-2)
> > {
> > @@ -2003,6 +2007,12 @@ struct vn_walk_cb_data
> > vn_lookup_kind vn_walk_kind;
> > bool tbaa_p;
> > bool redundant_store_removal_p;
> > + /* Whether the reference we are looking up is known to be a volatile
> > + access. */
> > + bool orig_volatile_p;
> > + /* Whether the original reference tree was unavailable, so that the
> > + volatility of the access could not be determined. */
> > + bool unknown_volatile_p;
> > vec<vn_reference_op_s> saved_operands;
> >
> > /* The VDEFs of partial defs we come along. */
> > @@ -2771,6 +2781,31 @@ adjust_offsets_for_equal_base_address (tree base1,
> > poly_int64 *offset1,
> > return operand_equal_p (base1, base2, OEP_ADDRESS_OF);
> > }
> >
> > +/* Return true if the value stored by DEF_STMT, a single assignment, may be
> > + forwarded to the reference DATA is looking up.
> > +
> > + A store determines the contents of the object it writes whether or not
> > it
> > + is performed through a volatile lvalue, so its value may be given to a
> > + later non-volatile load of the same location: either the object itself
> > is
> > + not volatile-qualified, in which case nothing else can change it, or it
> > is
> > + and reading it through a non-volatile lvalue is undefined. A volatile
> > + load has to access memory again, so no store may serve it (PR120944).
> > */
> > +
> > +static bool
> > +vn_stored_value_available_p (vn_walk_cb_data *data, gimple *def_stmt)
> > +{
> > + if (data->orig_volatile_p)
> > + return false;
> > +
> > + /* Without the original reference the volatility of the load is unknown,
> > + so keep to the narrower rule of refusing a store performed through a
> > + volatile lvalue. */
> > + if (data->unknown_volatile_p)
> > + return !TREE_THIS_VOLATILE (gimple_assign_lhs (def_stmt));
> > +
> > + return true;
> > +}
> > +
> > /* Callback for walk_non_aliased_vuses. Tries to perform a lookup
> > from the statement defining VUSE and if not successful tries to
> > translate *REFP and VR_ through an aggregate copy at the definition
> > @@ -3175,7 +3210,7 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void
> > *data_,
> > && gimple_assign_single_p (def_stmt)
> > && gimple_assign_rhs_code (def_stmt) == CONSTRUCTOR
> > && CONSTRUCTOR_NELTS (gimple_assign_rhs1 (def_stmt)) == 0
> > - && !TREE_THIS_VOLATILE (gimple_assign_lhs (def_stmt)))
> > + && vn_stored_value_available_p (data, def_stmt))
> > {
> > tree base2;
> > poly_int64 offset2, size2, maxsize2;
> > @@ -3231,7 +3266,7 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void
> > *data_,
> > && !reverse_storage_order_for_component_p (vr->operands)
> > && !contains_storage_order_barrier_p (vr->operands)
> > && gimple_assign_single_p (def_stmt)
> > - && !TREE_THIS_VOLATILE (gimple_assign_lhs (def_stmt))
> > + && vn_stored_value_available_p (data, def_stmt)
> > && CHAR_BIT == 8
> > && BITS_PER_UNIT == 8
> > && BYTES_BIG_ENDIAN == WORDS_BIG_ENDIAN
> > @@ -3396,7 +3431,7 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void
> > *data_,
> > && !reverse_storage_order_for_component_p (vr->operands)
> > && !contains_storage_order_barrier_p (vr->operands)
> > && gimple_assign_single_p (def_stmt)
> > - && !TREE_THIS_VOLATILE (gimple_assign_lhs (def_stmt))
> > + && vn_stored_value_available_p (data, def_stmt)
> > && TREE_CODE (gimple_assign_rhs1 (def_stmt)) == SSA_NAME)
> > {
> > tree lhs = gimple_assign_lhs (def_stmt);
> > @@ -4312,7 +4347,8 @@ vn_reference_lookup (tree op, tree vuse,
> > vn_lookup_kind kind,
> > }
> > vn_walk_cb_data data (&vr1, r.ref ? NULL_TREE : op,
> > last_vuse_ptr, kind, tbaa_p, mask,
> > - redundant_store_removal_p);
> > + redundant_store_removal_p,
> > + TREE_THIS_VOLATILE (op), false);
> >
> > wvnresult
> > = ((vn_reference_t)
> > --
> > 2.50.1 (Apple Git-155)
> >
>
--
Richard Biener <[email protected]>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Jochen Jaser, Andrew McDonald, Abhinav Puri; (HRB 36809, AG Nuernberg)
