This is my first stab at disallowing load data races that happen when we
cache the value of a global. I would appreciate input before
committing, especially on the RTL bits, cause it's been quite a while
since I typed d-e-b-u-g-_-r-t-x. :-)
In the example below we usually hoist "global" into a register or
temporary to avoid reading from it at each step. This would cause a
race if another thread had modified "global" in between iterations.
for (x=0; x< 5; x++)
sum[x] = global;
As expected, multiple passes can cause the same end result, so plugging
the problem involves multiple places. First, loop invariant movement
moves the invariant. Barring that, PRE moves the load, and even if we
get past the SSA passes, rtl-CSE pulls the rug from under us. If that
weren't enough, the post-reload pass performs CSE over the hard
registers, and we end up eliminating subsequent loads of "global". I am
sure there are many other places, but I'm starting with whatever it
takes to fix gcc.dg/memmodel/global-hoist.c.
The patch below fixes the test in question with "--param
allow-load-data-races=0". What do y'all think?
Thanks.
* tree.h (DECL_THREAD_VISIBLE_P): New.
* cselib.c (cselib_lookup_mem): Return 0 for globals for
restrictive memory model.
* cse.c (hash_rtx_cb): Same.
* gimple.h (gimple_has_thread_visible_ops): Protoize.
* gimple.c (gimple_has_thread_visible_ops): New.
* tree-ssa-loop-im.c (movement_possibility): Disallow when
avoiding load data races.
* tree-ssa-pre.c (compute_avail): Same, for globals.
Index: tree-ssa-loop-im.c
===================================================================
--- tree-ssa-loop-im.c (revision 170745)
+++ tree-ssa-loop-im.c (working copy)
@@ -377,6 +377,11 @@ movement_possibility (gimple stmt)
|| stmt_could_throw_p (stmt))
return MOVE_IMPOSSIBLE;
+ /* Do not move loads of globals if under a restrictive memory model. */
+ if (PARAM_VALUE (PARAM_ALLOW_LOAD_DATA_RACES) == 0
+ && gimple_has_thread_visible_ops (stmt))
+ return MOVE_IMPOSSIBLE;
+
if (is_gimple_call (stmt))
{
/* While pure or const call is guaranteed to have no side effects, we
Index: tree.h
===================================================================
--- tree.h (revision 170745)
+++ tree.h (working copy)
@@ -3102,6 +3102,10 @@ struct GTY(()) tree_parm_decl {
#define DECL_THREAD_LOCAL_P(NODE) \
(VAR_DECL_CHECK (NODE)->decl_with_vis.tls_model >= TLS_MODEL_REAL)
+/* Return true if a VAR_DECL is visible from another thread. */
+#define DECL_THREAD_VISIBLE_P(NODE) \
+ (TREE_STATIC (NODE) && !DECL_THREAD_LOCAL_P (NODE))
+
/* In a non-local VAR_DECL with static storage duration, true if the
variable has an initialization priority. If false, the variable
will be initialized at the DEFAULT_INIT_PRIORITY. */
Index: cse.c
===================================================================
--- cse.c (revision 170745)
+++ cse.c (working copy)
@@ -2402,6 +2402,19 @@ hash_rtx_cb (const_rtx x, enum machine_m
}
case MEM:
+ /* Avoid hoisting loads of globals if under a restrictive memory
+ model. */
+ if (PARAM_VALUE (PARAM_ALLOW_LOAD_DATA_RACES) == 0)
+ {
+ tree decl = MEM_EXPR (x);
+ if (do_not_record_p
+ && decl && TREE_CODE (decl) == VAR_DECL
+ && DECL_THREAD_VISIBLE_P (decl))
+ {
+ *do_not_record_p = 1;
+ return 0;
+ }
+ }
/* We don't record if marked volatile or if BLKmode since we don't
know the size of the move. */
if (do_not_record_p && (MEM_VOLATILE_P (x) || GET_MODE (x) == BLKmode))
Index: cselib.c
===================================================================
--- cselib.c (revision 170745)
+++ cselib.c (working copy)
@@ -1190,6 +1190,16 @@ cselib_lookup_mem (rtx x, int create)
cselib_val *mem_elt;
struct elt_list *l;
+ /* Forget any reads from globals if under a restrictive memory
+ model. */
+ if (PARAM_VALUE (PARAM_ALLOW_LOAD_DATA_RACES) == 0)
+ {
+ tree decl = MEM_EXPR (x);
+ if (decl && TREE_CODE (decl) == VAR_DECL
+ && DECL_THREAD_VISIBLE_P (decl))
+ return 0;
+ }
+
if (MEM_VOLATILE_P (x) || mode == BLKmode
|| !cselib_record_memory
|| (FLOAT_MODE_P (mode) && flag_float_store))
Index: tree-ssa-pre.c
===================================================================
--- tree-ssa-pre.c (revision 170745)
+++ tree-ssa-pre.c (working copy)
@@ -4000,6 +4000,12 @@ compute_avail (void)
|| stmt_could_throw_p (stmt))
continue;
+ /* Do not move loads of globals if under a restrictive
+ memory model. */
+ if (PARAM_VALUE (PARAM_ALLOW_LOAD_DATA_RACES) == 0
+ && gimple_has_thread_visible_ops (stmt))
+ continue;
+
switch (gimple_code (stmt))
{
case GIMPLE_RETURN:
Index: gimple.c
===================================================================
--- gimple.c (revision 170745)
+++ gimple.c (working copy)
@@ -2388,6 +2388,36 @@ gimple_rhs_has_side_effects (const_gimpl
return false;
}
+/* Return true if any of the operands in S are visible from another
+ thread (globals that are not TLS. */
+/* ?? If this becomes a performance penalty, we should cache this
+ value as we do with volatiles. */
+
+bool
+gimple_has_thread_visible_ops (const_gimple s)
+{
+ unsigned int i;
+
+ if (is_gimple_debug (s))
+ return false;
+
+ if (is_gimple_assign (s))
+ {
+ /* Skip the LHS. */
+ i = 1;
+ }
+ else
+ i = 0;
+ for (; i < gimple_num_ops (s); i++)
+ {
+ tree op = gimple_op (s, i);
+ if (op && TREE_CODE (op) == VAR_DECL
+ && DECL_THREAD_VISIBLE_P (op))
+ return true;
+ }
+ return false;
+}
+
/* Helper for gimple_could_trap_p and gimple_assign_rhs_could_trap_p.
Return true if S can trap. When INCLUDE_MEM is true, check whether
the memory operations could trap. When INCLUDE_STORES is true and
Index: gimple.h
===================================================================
--- gimple.h (revision 170745)
+++ gimple.h (working copy)
@@ -882,6 +882,7 @@ gimple gimple_build_cond_from_tree (tree
void gimple_cond_set_condition_from_tree (gimple, tree);
bool gimple_has_side_effects (const_gimple);
bool gimple_rhs_has_side_effects (const_gimple);
+bool gimple_has_thread_visible_ops (const_gimple);
bool gimple_could_trap_p (gimple);
bool gimple_could_trap_p_1 (gimple, bool, bool);
bool gimple_assign_rhs_could_trap_p (gimple);
