> On 26 Aug 2026, at 08:31, Richard Biener <[email protected]> wrote: > > 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. >
Yeah, I see. I suppose we could raise it with git, though I suspect a counter would be that “Clang gets the better codegen” and we’d be arguing from a weaker position. FWIW the relevant code is the sha1_store macro in: https://github.com/git/git/blob/master/sha1dc/sha1.c Thanks, Kyrill > 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)
