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