From patchwork Thu Mar 24 17:33:05 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 88256 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 708B11007D1 for ; Fri, 25 Mar 2011 04:33:19 +1100 (EST) Received: (qmail 30945 invoked by alias); 24 Mar 2011 17:33:17 -0000 Received: (qmail 30934 invoked by uid 22791); 24 Mar 2011 17:33:15 -0000 X-SWARE-Spam-Status: No, hits=-6.5 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_HI, SPF_HELO_PASS, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 24 Mar 2011 17:33:06 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p2OHX6Ad007632 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 24 Mar 2011 13:33:06 -0400 Received: from houston.quesejoda.com (vpn-229-62.phx2.redhat.com [10.3.229.62]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p2OHX53T006926; Thu, 24 Mar 2011 13:33:06 -0400 Message-ID: <4D8B8051.2030307@redhat.com> Date: Thu, 24 Mar 2011 12:33:05 -0500 From: Aldy Hernandez User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.15) Gecko/20110307 Fedora/3.1.9-0.39.b3pre.fc14 Lightning/1.0b3pre Thunderbird/3.1.9 MIME-Version: 1.0 To: gcc-patches , Jeff Law , Richard Henderson Subject: [cxx-mem-model] disallow load data races (1 of some) Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org 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);