From patchwork Mon Oct 15 12:28:15 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 191553 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 CBAB62C00A3 for ; Mon, 15 Oct 2012 23:28:35 +1100 (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=1350908917; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Message-ID:Date:From:User-Agent:MIME-Version:To:Subject: Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:Sender:Delivered-To; bh=MFp4d0/ +uDdJjD+n6AVvMJ/L3pE=; b=pVhLnaua+gh/B7DtZAzjZgFbPS+vSdrNOUYOPkX 8y+ToHC/HBWL8j4bh75du9pMxyUArcr371ajViPQ4vg4LyzVVcCOUaXOJsSgkMEX cld4lLQZnNschG/d9BpLXOPESnj87mfM4m+FI8uyy/w0IeejGEJ71wj3EzuLYWCS Qm1c= 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:Subject:Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=fZ/M24cQSYvMiiNiJDqC00C36q7kSfxyKd4ilcPS4GnbvNkxTQ/N3pdO8d6leG V6MIT+IGKr09kzcIIibcM9n1ywB8H3Yq3nCifcraa4qgrtfFylaOncDm9EijSYKj VHeaLtVmDavnoXQgE5gGOA5OIeklhjtAxTDF9nuWNPo2M=; Received: (qmail 19293 invoked by alias); 15 Oct 2012 12:28:31 -0000 Received: (qmail 19278 invoked by uid 22791); 15 Oct 2012 12:28:28 -0000 X-SWARE-Spam-Status: No, hits=-6.5 required=5.0 tests=AWL, BAYES_00, KHOP_RCVD_UNTRUST, RCVD_IN_DNSWL_HI, RCVD_IN_HOSTKARMA_W, RP_MATCHES_RCVD, SPF_HELO_PASS 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; Mon, 15 Oct 2012 12:28:20 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q9FCSIYI018395 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 15 Oct 2012 08:28:18 -0400 Received: from reynosa.quesejoda.com (vpn-11-62.rdu.redhat.com [10.11.11.62]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q9FCSG4C015391; Mon, 15 Oct 2012 08:28:16 -0400 Message-ID: <507C015F.2090508@redhat.com> Date: Mon, 15 Oct 2012 08:28:15 -0400 From: Aldy Hernandez User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120911 Thunderbird/15.0.1 MIME-Version: 1.0 To: gcc-patches , Richard Guenther , Ian Lance Taylor , Jakub Jelinek , Andrew MacLeod Subject: [path] PR 54900: store data race in if-conversion pass 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 [Ian, I am copying you because this is originally your code. Richard, I am copying you because you are all things aliased :-). And Andrew is here because I am unable to come up with a suitable test for the simulate-thread harness.]. Here we have a store data race because noce_can_store_speculate_p() incorrectly returns true. The problem is that memory_modified_in_insn_p() returns true if an instruction *MAY* modify a memory location, but the store can only be speculated if we are absolutely sure the store will happen on all subsequent paths. My approach is to implement a memory_SURELY_modified_in_insn_p(), which will trigger this optimization less often, but at least it will be correct. I thought of restricting the speculation for "--param allow-store-data-race=0" or transactional code, but IMO the speculation is incorrect as is. I am having a bit of a problem coming up with a generic testcase. Perhaps Andrew or others have an idea. The attached testcase fails to trigger without the patch, because AFAICT we have no way of testing an addition of zero to a memory location: cmpl $1, flag(%rip) setb %al addl %eax, dont_write(%rip) In the simulate-thread harness I can test the environment before an instruction, and after an instruction, but adding 0 to *dont_write produces no measurable effects, particularly in a back-end independent manner. Ideas? Bootstrap and regtested on x86-64 Linux. Patch OK? (Except the test itself.) PR tree-optimization/54900 * ifcvt.c (noce_can_store_speculate_p): Call memory_surely_modified_in_insn_p. * alias.c (memory_surely_modified_in_insn_p): New. (memory_modified_in_insn_p): Change comment. diff --git a/gcc/alias.c b/gcc/alias.c index 0c6a744..26d3797 100644 --- a/gcc/alias.c +++ b/gcc/alias.c @@ -2748,8 +2748,8 @@ memory_modified_1 (rtx x, const_rtx pat ATTRIBUTE_UNUSED, void *data) } -/* Return true when INSN possibly modify memory contents of MEM - (i.e. address can be modified). */ +/* Return true if INSN *MAY* possibly modify the memory contents of + MEM (i.e. address can be modified). */ bool memory_modified_in_insn_p (const_rtx mem, const_rtx insn) { @@ -2760,6 +2760,22 @@ memory_modified_in_insn_p (const_rtx mem, const_rtx insn) return memory_modified; } +/* Like memory_modified_in_insn_p, but return TRUE if INSN will + *SURELY* modify the memory contents of MEM. */ +bool +memory_surely_modified_in_insn_p (const_rtx mem, const_rtx insn) +{ + if (!INSN_P (insn)) + return false; + rtx set = single_set (insn); + if (set) + { + rtx dest = SET_DEST (set); + return rtx_equal_p (dest, mem); + } + return false; +} + /* Initialize the aliasing machinery. Initialize the REG_KNOWN_VALUE array. */ diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index 2f486a2..659e464 100644 --- a/gcc/ifcvt.c +++ b/gcc/ifcvt.c @@ -2415,7 +2415,7 @@ noce_can_store_speculate_p (basic_block top_bb, const_rtx mem) || (CALL_P (insn) && (!RTL_CONST_CALL_P (insn))))) return false; - if (memory_modified_in_insn_p (mem, insn)) + if (memory_surely_modified_in_insn_p (mem, insn)) return true; if (modified_in_p (XEXP (mem, 0), insn)) return false; diff --git a/gcc/rtl.h b/gcc/rtl.h index cd5d435..d449ee1 100644 --- a/gcc/rtl.h +++ b/gcc/rtl.h @@ -2614,6 +2614,7 @@ extern void init_alias_analysis (void); extern void end_alias_analysis (void); extern void vt_equate_reg_base_value (const_rtx, const_rtx); extern bool memory_modified_in_insn_p (const_rtx, const_rtx); +extern bool memory_surely_modified_in_insn_p (const_rtx, const_rtx); extern bool may_be_sp_based_p (rtx); extern rtx gen_hard_reg_clobber (enum machine_mode, unsigned int); extern rtx get_reg_known_value (unsigned int); diff --git a/gcc/testsuite/gcc.dg/simulate-thread/speculative-store-5.c b/gcc/testsuite/gcc.dg/simulate-thread/speculative-store-5.c new file mode 100644 index 0000000..52daa27 --- /dev/null +++ b/gcc/testsuite/gcc.dg/simulate-thread/speculative-store-5.c @@ -0,0 +1,63 @@ +/* { dg-do link } */ +/* { dg-options "--param allow-store-data-races=0" } */ +/* { dg-final { simulate-thread } } */ + +#include +#include "simulate-thread.h" + +/* PR tree-optimization/54900 */ + +/* This file tests that a speculative store does not happen unless we + are sure a store to the location would happen anyway. */ + +int flag = 1; +int stuff; +int *stuffp = &stuff; +int dont_write = 555; + +void simulate_thread_other_threads() +{ +} + +int simulate_thread_step_verify() +{ + if (dont_write != 555) + { + printf("FAIL: dont_write variable was assigned to. \n"); + return 1; + } + return 0; +} + +int simulate_thread_final_verify() +{ + return 0; +} + +void outerfunc (int p1) +{ + *stuffp = 0; +} + +int innerfunc () +{ + if (flag) + return 0; + /* This store should never happen because flag is globally set to true. */ + ++dont_write; + return 0; +} + +__attribute__((noinline)) +void simulate_thread_main() +{ + outerfunc (innerfunc ()); + simulate_thread_done(); +} + +__attribute__((noinline)) +int main() +{ + simulate_thread_main(); + return 0; +}