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