From patchwork Mon Jan 2 18:10:19 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Torvald Riegel X-Patchwork-Id: 133896 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 1523AB6FA5 for ; Tue, 3 Jan 2012 05:10:51 +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=1326132652; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Subject:From:To:Content-Type:Date:Message-ID:Mime-Version: Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:Sender:Delivered-To; bh=bGPDNCRPgVW+4r7Zfr5U NUCdL/4=; b=jr1IkL8fQCeCHfNivGtNT01hZuI8J2QsyY1DR1vwzl41ADgysqU+ sM2q8H4YQdwYyW+QtBAltdCw3fIFsofkU4MR8xZp9eHEHuqg95sql9eIRzB71Hc9 cc4PNkM5kkfEKXvqDjWRRyy5WU5vAlBWRSoFUTg0E7cvTF0661LKSZc= 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:Subject:From:To:Content-Type:Date:Message-ID:Mime-Version:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=BNXv21Zw9zvHTKBvcgknI47Q+IxrmnAMf1lkTYHt3jvjbbMkNiPiZOKkZpdSoj /vEEiwfakY1R9OlILSyUQXmX+Xf0P21ysey1GgBk3KpZHO7N6Obzc8tZfaG9489i Ghg/A3E8DIrsvwAoIBRpMw0R9DqB0Hh5FBNGeOOdnIvik=; Received: (qmail 30485 invoked by alias); 2 Jan 2012 18:10:46 -0000 Received: (qmail 30477 invoked by uid 22791); 2 Jan 2012 18:10:44 -0000 X-SWARE-Spam-Status: No, hits=-7.2 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, SPF_HELO_PASS, TW_GJ, TW_TJ 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, 02 Jan 2012 18:10:22 +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 q02IAMX8032033 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 2 Jan 2012 13:10:22 -0500 Received: from [10.36.5.32] (vpn1-5-32.ams2.redhat.com [10.36.5.32]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q02IAKN7010113; Mon, 2 Jan 2012 13:10:21 -0500 Subject: [RFC][patch] trans-mem: mark transaction begins as returns-twice From: Torvald Riegel To: GCC Patches , Richard Henderson , Aldy Hernandez Date: Mon, 02 Jan 2012 19:10:19 +0100 Message-ID: <1325527819.7636.552.camel@triegel.csb> Mime-Version: 1.0 X-IsSubscribed: yes 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 Attached is Richard Henderson's patch that marks calls that start a transaction as returns-twice. In turn, these calls will be treated like a call to setjmp. This was motivated by the miscompilation of one of the STAMP applications (Genome), where a stack slot was used as temp storage for a CPU register but not restored when the transaction got aborted and restarted (then, after restart, the program crashed because it used inconsistent data). With the attached patch and in this particular example, the stack slots that are written to in the transaction do not get read during the transaction. (-fno-caller-saves was not a sufficient solution, BTW.) AFAICT, we previously wanted to handle "restart safety" by adding abnormal edges to all calls to the TM runtime library (which could in turn call the libitm longjmp that actually restarts a transaction). Richard mentioned that we could drop this code (I think he meant this, and others pieces perhaps) if the returns-twice approach works. BTW, did we also add abnormal edges to any call to transactional clones, which could in turn call the TM runtime library? Conceptually, what we would need to enforce is that all stack slots that are live-in into a transaction (i.e., the live range crosses a call to _ITM_beginTransaction) do not get reused until the matching call to _ITM_commitTransaction. Alternatively, we can reuse those, but need to save and restore them after aborts. As Richard suggested, I looked at the handling of REG_SETJMP (see the summary below). This looks like a sufficient solution to me (eg, don't share stack slots if regs cross a setjmp) but I don't know nearly enough about this to really understand whether it is. Can somebody else have a look please? In particular: - Is it really sufficient? - Is this too conservative (e.g., no CSE at all)? If so, we should look at this again for 4.8 I guess, otherwise every function that uses a transaction will be slower, and slowing down nontransactional code isn't nice. Or won't that be much of a slowdown? - Do we potentially get unnecessary warnings (if vars are live across a transaction begin)? I didn't get such warnings in the STAMP app that wasn't working though. Does anyone has suggestions for a test case? - For the long-term, should we try to limit the setjmp effects to the TM region? Contrary to standard setjmp/longjmp, we know about the regions and where the longjmps matching a setjmp are. Any thoughts on how much this might matter performance-wise? Overall, I think we should still use it, unless we can come up with another fix in time for 4.7. Opinions? Torvald Summary: - cprop.c: constant propagation only runs if !cfun->calls_setjmp - gcse.c: Same. - cse.c: we don't CSE over a call to setjmp. But comment associates this just with some weird longjmp on VAX and other systems. - cselib.c (cselib_process_insn): Forget everything at a setjmp. - callers of this function are: dse_step1(), local_cprop_pass(), reload_cse_regs(), thread_jump(), vt_initialize() - ira-lives.c (process_bb_node_lives): ??? Don't allocate allocnos that cross setjmps. - regstat.c (regstat_bb_compute_ri): Marks all pseudo(?) regs that cross a setjmp. Clients of this information: - (regstat_compute_ri): ??? REG_LIVE_LENGTH(regno) = -1; REG_BASIC_BLOCK(regno) = REG_BLOCK_UNKNOWN; - function.c (generate_setjmp_warnings): Warnings if vars are live across setjmp. - ira-color.c (coalesce_spill_slots): ??? Don't share stack slots if one of the regs crosses a setjmp? - reload1.c (reload_as_needed): Don't reuse any reload reg contents across a setjmp. - reload.c (find_equiv_reg): Don't reuse register contents from before a setjmp-type function call. - resource.c (mark_referenced_resources, mark_set_resources): setjmp uses all registers. - sched-deps.c: setjmp acts as a MOVE_BARRIER. What does this barrier do precisely? commit 53e0ab5b86ca38d55e4c4fd927be33143586ad15 Author: Torvald Riegel Date: Mon Dec 19 23:36:45 2011 +0100 rth's returns-twice fix diff --git a/gcc/builtin-attrs.def b/gcc/builtin-attrs.def index 619794e..c3132cc 100644 --- a/gcc/builtin-attrs.def +++ b/gcc/builtin-attrs.def @@ -98,6 +98,7 @@ DEF_ATTR_IDENT (ATTR_STRFTIME, "strftime") DEF_ATTR_IDENT (ATTR_TYPEGENERIC, "type generic") DEF_ATTR_IDENT (ATTR_TM_REGPARM, "*tm regparm") DEF_ATTR_IDENT (ATTR_TM_TMPURE, "transaction_pure") +DEF_ATTR_IDENT (ATTR_RETURNS_TWICE, "returns_twice") DEF_ATTR_TREE_LIST (ATTR_NOVOPS_LIST, ATTR_NOVOPS, ATTR_NULL, ATTR_NULL) @@ -241,6 +242,8 @@ DEF_ATTR_TREE_LIST (ATTR_TM_NORETURN_NOTHROW_LIST, ATTR_TM_REGPARM, ATTR_NULL, ATTR_NORETURN_NOTHROW_LIST) DEF_ATTR_TREE_LIST (ATTR_TM_CONST_NOTHROW_LIST, ATTR_TM_REGPARM, ATTR_NULL, ATTR_CONST_NOTHROW_LIST) +DEF_ATTR_TREE_LIST (ATTR_TM_NOTHROW_RT_LIST, + ATTR_RETURNS_TWICE, ATTR_NULL, ATTR_TM_NOTHROW_LIST) /* Same attributes used for BUILT_IN_MALLOC except with TM_PURE thrown in. */ DEF_ATTR_TREE_LIST (ATTR_TMPURE_MALLOC_NOTHROW_LIST, diff --git a/gcc/gtm-builtins.def b/gcc/gtm-builtins.def index 9fcbdb0..1630a0e 100644 --- a/gcc/gtm-builtins.def +++ b/gcc/gtm-builtins.def @@ -1,5 +1,5 @@ DEF_TM_BUILTIN (BUILT_IN_TM_START, "_ITM_beginTransaction", - BT_FN_UINT_UINT, ATTR_TM_NOTHROW_LIST) + BT_FN_UINT_UINT, ATTR_TM_NOTHROW_RT_LIST) DEF_TM_BUILTIN (BUILT_IN_TM_COMMIT, "_ITM_commitTransaction", BT_FN_VOID, ATTR_TM_NOTHROW_LIST)