> Am 26.08.2026 um 16:19 schrieb Kyrylo Tkachov <[email protected]>:
>
>
>
>> 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
It seems to me this is either premature optimization against register pressure
(you’d defeat this with your patch) or working around TBAA issues in the code.
So very much to be reported against git.
Richard
> 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)
>
>
