From patchwork Thu Apr 12 22:11:48 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 152195 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 3191DB70B0 for ; Fri, 13 Apr 2012 08:12:14 +1000 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1334873535; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Message-ID:Date:From:User-Agent:MIME-Version:To:CC:Subject: Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:Sender:Delivered-To; bh=OzpURPx bQRz4+7gqKOyEBz2wp4Q=; b=pWG4fUg3WkGmhhYhmyHpJhVYjHj38cmszAodTtb VA9FgAPzGrwFugfjj0XZ7xHx/5YFko3k7OkYDATg7aSEbOoF9hZdjJuGvbAzxjGT QNt2LzGzPnI84zJVOLFyJHNEEKgPcJLB6od+7n+Wt4mDCc7H17T5FjYv1XOGrdB4 lDoE= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:Received:Message-ID:Date:From:User-Agent:MIME-Version:To:CC:Subject:Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=e+kHz4nLQdOtyAV7XuSMDM6M2CX+aw5euwCxVcyCszIDNpQ64J/UVOoWJ0BKVN doAvzmLvHAL1omq39OM5XTa8DYAlD7byYO8Dh0228P4x9p7Il4X7kq5cT34tRy3t JZvWkwAe8D7G1Jcj9WVsOpryg+Ch+9cSaNocVN9/5w7ko=; Received: (qmail 23920 invoked by alias); 12 Apr 2012 22:12:10 -0000 Received: (qmail 23912 invoked by uid 22791); 12 Apr 2012 22:12:08 -0000 X-SWARE-Spam-Status: No, hits=-6.3 required=5.0 tests=AWL, BAYES_00, KHOP_RCVD_UNTRUST, RCVD_IN_DNSWL_HI, RCVD_IN_HOSTKARMA_W, SPF_HELO_PASS, TW_TM, 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, 12 Apr 2012 22:11:49 +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 q3CMBnvR021020 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 12 Apr 2012 18:11:49 -0400 Received: from houston.quesejoda.com (vpn-225-239.phx2.redhat.com [10.3.225.239]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q3CMBm6a029123; Thu, 12 Apr 2012 18:11:48 -0400 Message-ID: <4F875324.7060908@redhat.com> Date: Thu, 12 Apr 2012 17:11:48 -0500 From: Aldy Hernandez User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20120329 Thunderbird/11.0.1 MIME-Version: 1.0 To: Richard Guenther CC: Andrew MacLeod , "Boehm, Hans" , gcc-patches , Torvald Riegel Subject: [PR tree-optimization/52558]: RFC: questions on store data race 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 Here we have a testcase that affects both the C++ memory model and transactional memory. [Hans, this is caused by the same problem that is causing the speculative register promotion issue you and Torvald pointed me at]. In the following testcase (adapted from the PR), the loop invariant motion pass caches a pre-existing value for g_2, and then performs a store to g_2 on every path, causing a store data race: int g_1 = 1; int g_2 = 0; int func_1(void) { int l; for (l = 0; l < 1234; l++) { if (g_1) return l; else g_2 = 0; <-- Store to g_2 should only happen if !g_1 } return 999; } This gets transformed into something like: g_2_lsm = g_2; if (g_1) { g_2 = g_2_lsm; // boo! hiss! return 0; } else { g_2_lsm = 0; g_2 = g_2_lsm; } The spurious write to g_2 could cause a data race. Andrew has suggested verifying that the store to g_2 was actually different than on entry, and letting PHI copy propagation optimize the redundant comparisons. Like this: g_2_lsm = g_2; if (g_1) { if (g_2_lsm != g_2) // hopefully optimized away g_2 = g_2_lsm; // hopefully optimized away return 0; } else { g_2_lsm = 0; if (g_2_lsm != g_2) // hopefully optimized away g_2 = g_2_lsm; } ...which PHI copy propagation and/or friends should be able to optimize. For that matter, regardless of the memory model, the proposed solution would improve the existing code by removing the extra store (in this contrived test case anyhow). Anyone see a problem with this approach (see attached patch)? Assuming the unlikely scenario that everyone likes this :), I have two problems that I would like answered. But feel free to ignore if the approach is a no go. 1. No pass can figure out the equality (or inequality) of g_2_lsm and g_2. In the PR, Richard mentions that both FRE/PRE can figure this out, but they are not run after store motion. DOM should also be able to, but apparently does not :(. Tips? 2. The GIMPLE_CONDs I create in this patch are currently causing problems with complex floats/doubles. do_jump is complaining that it can't compare them, so I assume it is too late (in tree-ssa-loop-im.c) to compare complex values since complex lowering has already happened (??). Is there a more canonical way of creating a GIMPLE_COND that may contain complex floats at this stage? Thanks folks. * tree-ssa-loop-im.c (execute_sm): Guard store motion with a conditional. * opts.c (finish_options): Do not allow store or load data races with -fgnu-tm. Index: tree-ssa-loop-im.c =================================================================== --- tree-ssa-loop-im.c (revision 186108) +++ tree-ssa-loop-im.c (working copy) @@ -1999,8 +1999,59 @@ execute_sm (struct loop *loop, VEC (edge FOR_EACH_VEC_ELT (edge, exits, i, ex) { - store = gimple_build_assign (unshare_expr (ref->mem), tmp_var); - gsi_insert_on_edge (ex, store); + basic_block new_bb, then_bb, old_dest; + edge then_old_edge; + gimple_stmt_iterator gsi; + gimple stmt; + tree t1; + + if (PARAM_VALUE (PARAM_ALLOW_STORE_DATA_RACES)) + { + store = gimple_build_assign (unshare_expr (ref->mem), tmp_var); + gsi_insert_on_edge (ex, store); + } + else + { + old_dest = ex->dest; + new_bb = split_edge (ex); + then_bb = create_empty_bb (new_bb); + if (current_loops && new_bb->loop_father) + add_bb_to_loop (then_bb, new_bb->loop_father); + + gsi = gsi_start_bb (new_bb); + t1 = make_rename_temp (TREE_TYPE (ref->mem), NULL); + stmt = gimple_build_assign (t1, unshare_expr (ref->mem)); + gsi_insert_after (&gsi, stmt, GSI_CONTINUE_LINKING); + stmt = gimple_build_cond (NE_EXPR, t1, tmp_var, + NULL_TREE, NULL_TREE); + gsi_insert_after (&gsi, stmt, GSI_CONTINUE_LINKING); + + gsi = gsi_start_bb (then_bb); + store = gimple_build_assign (unshare_expr (ref->mem), tmp_var); + gsi_insert_after (&gsi, store, GSI_CONTINUE_LINKING); + + make_edge (new_bb, then_bb, EDGE_TRUE_VALUE); + make_edge (new_bb, old_dest, EDGE_FALSE_VALUE); + then_old_edge = make_edge (then_bb, old_dest, EDGE_FALLTHRU); + set_immediate_dominator (CDI_DOMINATORS, then_bb, new_bb); + + for (gsi = gsi_start_phis (old_dest); !gsi_end_p (gsi); gsi_next (&gsi)) + { + gimple phi = gsi_stmt (gsi); + unsigned i; + + for (i = 0; i < gimple_phi_num_args (phi); i++) + if (gimple_phi_arg_edge (phi, i)->src == new_bb) + { + tree arg = gimple_phi_arg_def (phi, i); + add_phi_arg (phi, arg, then_old_edge, UNKNOWN_LOCATION); + update_stmt (phi); + } + } + /* Remove the original fall through edge. This was the + single_succ_edge (new_bb). */ + EDGE_SUCC (new_bb, 0)->flags &= ~EDGE_FALLTHRU; + } } } Index: opts.c =================================================================== --- opts.c (revision 186108) +++ opts.c (working copy) @@ -663,8 +663,16 @@ finish_options (struct gcc_options *opts opts->x_flag_toplevel_reorder = 0; } - if (opts->x_flag_tm && opts->x_flag_non_call_exceptions) - sorry ("transactional memory is not supported with non-call exceptions"); + if (opts->x_flag_tm) + { + if (opts->x_flag_non_call_exceptions) + sorry ("transactional memory is not supported with non-call exceptions"); + + set_param_value ("allow-load-data-races", 0, + opts->x_param_values, opts_set->x_param_values); + set_param_value ("allow-store-data-races", 0, + opts->x_param_values, opts_set->x_param_values); + } /* -Wmissing-noreturn is alias for -Wsuggest-attribute=noreturn. */ if (opts->x_warn_missing_noreturn)