From patchwork Wed Jan 14 15:44:13 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew MacLeod X-Patchwork-Id: 428999 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]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4623914015A for ; Thu, 15 Jan 2015 02:44:33 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; q=dns; s=default; b=D9LQP1ceX8eXXKqbZ VCK6mi1mqesgThkJj8POmm4ywExHrXlEckfmLrL5Rb3HO+b1ju+nd2DaRiyL0e9n l1R2JbuPPvlAvjyWbjtXOk2kGwy6k4+63KbM5Du8GoESGbrHyL0iU/7KeG3cxxGN DJFQTO5jnywY5CTXe7Zc5RHLx8= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; s=default; bh=TMOqgVT+0j6y8Hu3wE9ENdc MOAw=; b=iDZJxj40JeNQHjmj5FOkMgFW4YNIQ6po5kaXFUpLMTO0roRxTHv0/vl 2e4VQV9sP2gUTaWJCOiUn5dAC1dokv+F43xLCVzyB8KAKaGZnWdH7I66PdEAMbcR UvF8W2bHHO6lRfuSQIKq6Nsr2P9pvrnQxRi259SL2Zwfqbp7c+7s= Received: (qmail 10020 invoked by alias); 14 Jan 2015 15:44:26 -0000 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 Received: (qmail 10010 invoked by uid 89); 14 Jan 2015 15:44:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 14 Jan 2015 15:44:19 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t0EFiFuG032250 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 14 Jan 2015 10:44:15 -0500 Received: from [10.10.57.179] (vpn-57-179.rdu2.redhat.com [10.10.57.179]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t0EFiDkX011883; Wed, 14 Jan 2015 10:44:14 -0500 Message-ID: <54B68ECD.5070409@redhat.com> Date: Wed, 14 Jan 2015 10:44:13 -0500 From: Andrew MacLeod User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: Joseph Myers CC: Torvald Riegel , Richard Biener , gcc-patches , Jeff Law Subject: Re: [PATCH] PR59448 - Promote consume to acquire References: <54B53203.6030304@redhat.com> <54B53587.9080803@redhat.com> <1421174335.17929.7.camel@triegel.csb> <54B56CB0.7000902@redhat.com> In-Reply-To: X-IsSubscribed: yes On 01/13/2015 05:37 PM, Joseph Myers wrote: > On Tue, 13 Jan 2015, Andrew MacLeod wrote: > >> It seems that it should be safe to move back to the original patch, and remove >> that error test for using consume on an exchange... > I don't think there should be any such errors, for any of the atomic > built-in functions, only warnings (with the model converted to > MEMMODEL_SEQ_CST if not valid, just like a non-constant model). This is > just like any other invalid function argument of a suitable type: > undefined behavior only if the call is executed. > > http://www.open-std.org/jtc1/sc22/wg14/12553 > Can't see what is in the link, but we've discussed this before. - There is a warning for invalid memory models already, so I just continued using that. - I remove the check for CONSUME in exchange since the current standard makes no mention of that being illegal. - I also reversed the current check in compare_exchange to check for failure > success first, allowing us to still catch both errors if present. I think this brings us to where we ought to be... at least almost :-) The latest version I have is n3337, which still specifies that atomic_clear can't be memory_order_acquire or memory_order_acq_rel. Has that been updated to specify that memory_order_consume is not allowed either? I think there was a request in at some point... I can add that if so. Bootstraps on x86_64-unknown-linux-gnu, and no regressions in the testsuite. OK for trunk? Andrew 2015-01-14 Andrew MacLeod * builtins.c (expand_builtin_atomic_exchange): Remove error when memory model is CONSUME. (expand_builtin_atomic_compare_exchange, expand_builtin_atomic_load, expand_builtin_atomic_store, expand_builtin_atomic_clear): Change invalid memory model errors to warnings. * testsuite/gcc.dg/atomic-invalid.c: Check for invalid memory model warnings instead of errors. Index: builtins.c =================================================================== *** builtins.c (revision 219601) --- builtins.c (working copy) *************** expand_builtin_atomic_exchange (machine_ *** 5385,5395 **** enum memmodel model; model = get_memmodel (CALL_EXPR_ARG (exp, 2)); - if ((model & MEMMODEL_MASK) == MEMMODEL_CONSUME) - { - error ("invalid memory model for %<__atomic_exchange%>"); - return NULL_RTX; - } if (!flag_inline_atomics) return NULL_RTX; --- 5385,5390 ---- *************** expand_builtin_atomic_compare_exchange ( *** 5422,5441 **** success = get_memmodel (CALL_EXPR_ARG (exp, 4)); failure = get_memmodel (CALL_EXPR_ARG (exp, 5)); if ((failure & MEMMODEL_MASK) == MEMMODEL_RELEASE || (failure & MEMMODEL_MASK) == MEMMODEL_ACQ_REL) { ! error ("invalid failure memory model for %<__atomic_compare_exchange%>"); ! return NULL_RTX; } ! if (failure > success) ! { ! error ("failure memory model cannot be stronger than success " ! "memory model for %<__atomic_compare_exchange%>"); ! return NULL_RTX; ! } ! if (!flag_inline_atomics) return NULL_RTX; --- 5417,5441 ---- success = get_memmodel (CALL_EXPR_ARG (exp, 4)); failure = get_memmodel (CALL_EXPR_ARG (exp, 5)); + if (failure > success) + { + warning (OPT_Winvalid_memory_model, + "failure memory model cannot be stronger than success memory " + "model for %<__atomic_compare_exchange%>"); + success = MEMMODEL_SEQ_CST; + } + if ((failure & MEMMODEL_MASK) == MEMMODEL_RELEASE || (failure & MEMMODEL_MASK) == MEMMODEL_ACQ_REL) { ! warning (OPT_Winvalid_memory_model, ! "invalid failure memory model for " ! "%<__atomic_compare_exchange%>"); ! failure = MEMMODEL_SEQ_CST; ! success = MEMMODEL_SEQ_CST; } ! if (!flag_inline_atomics) return NULL_RTX; *************** expand_builtin_atomic_load (machine_mode *** 5491,5498 **** if ((model & MEMMODEL_MASK) == MEMMODEL_RELEASE || (model & MEMMODEL_MASK) == MEMMODEL_ACQ_REL) { ! error ("invalid memory model for %<__atomic_load%>"); ! return NULL_RTX; } if (!flag_inline_atomics) --- 5491,5499 ---- if ((model & MEMMODEL_MASK) == MEMMODEL_RELEASE || (model & MEMMODEL_MASK) == MEMMODEL_ACQ_REL) { ! warning (OPT_Winvalid_memory_model, ! "invalid memory model for %<__atomic_load%>"); ! model = MEMMODEL_SEQ_CST; } if (!flag_inline_atomics) *************** expand_builtin_atomic_store (machine_mod *** 5521,5528 **** && (model & MEMMODEL_MASK) != MEMMODEL_SEQ_CST && (model & MEMMODEL_MASK) != MEMMODEL_RELEASE) { ! error ("invalid memory model for %<__atomic_store%>"); ! return NULL_RTX; } if (!flag_inline_atomics) --- 5522,5530 ---- && (model & MEMMODEL_MASK) != MEMMODEL_SEQ_CST && (model & MEMMODEL_MASK) != MEMMODEL_RELEASE) { ! warning (OPT_Winvalid_memory_model, ! "invalid memory model for %<__atomic_store%>"); ! model = MEMMODEL_SEQ_CST; } if (!flag_inline_atomics) *************** expand_builtin_atomic_clear (tree exp) *** 5628,5635 **** if ((model & MEMMODEL_MASK) == MEMMODEL_ACQUIRE || (model & MEMMODEL_MASK) == MEMMODEL_ACQ_REL) { ! error ("invalid memory model for %<__atomic_store%>"); ! return const0_rtx; } if (HAVE_atomic_clear) --- 5630,5638 ---- if ((model & MEMMODEL_MASK) == MEMMODEL_ACQUIRE || (model & MEMMODEL_MASK) == MEMMODEL_ACQ_REL) { ! warning (OPT_Winvalid_memory_model, ! "invalid memory model for %<__atomic_store%>"); ! model = MEMMODEL_SEQ_CST; } if (HAVE_atomic_clear) Index: testsuite/gcc.dg/atomic-invalid.c =================================================================== *** testsuite/gcc.dg/atomic-invalid.c (revision 219601) --- testsuite/gcc.dg/atomic-invalid.c (working copy) *************** bool x; *** 13,35 **** int main () { ! __atomic_compare_exchange_n (&i, &e, 1, 0, __ATOMIC_RELAXED, __ATOMIC_SEQ_CST); /* { dg-error "failure memory model cannot be stronger" } */ ! __atomic_compare_exchange_n (&i, &e, 1, 0, __ATOMIC_SEQ_CST, __ATOMIC_RELEASE); /* { dg-error "invalid failure memory" } */ ! __atomic_compare_exchange_n (&i, &e, 1, 1, __ATOMIC_SEQ_CST, __ATOMIC_ACQ_REL); /* { dg-error "invalid failure memory" } */ ! ! __atomic_load_n (&i, __ATOMIC_RELEASE); /* { dg-error "invalid memory model" } */ ! __atomic_load_n (&i, __ATOMIC_ACQ_REL); /* { dg-error "invalid memory model" } */ ! ! __atomic_store_n (&i, 1, __ATOMIC_ACQUIRE); /* { dg-error "invalid memory model" } */ ! __atomic_store_n (&i, 1, __ATOMIC_CONSUME); /* { dg-error "invalid memory model" } */ ! __atomic_store_n (&i, 1, __ATOMIC_ACQ_REL); /* { dg-error "invalid memory model" } */ i = __atomic_always_lock_free (s, NULL); /* { dg-error "non-constant argument" } */ __atomic_load_n (&i, 44); /* { dg-warning "invalid memory model" } */ ! __atomic_clear (&x, __ATOMIC_ACQUIRE); /* { dg-error "invalid memory model" } */ ! __atomic_clear (&x, __ATOMIC_ACQ_REL); /* { dg-error "invalid memory model" } */ } --- 13,35 ---- int main () { ! __atomic_compare_exchange_n (&i, &e, 1, 0, __ATOMIC_RELAXED, __ATOMIC_SEQ_CST); /* { dg-warning "failure memory model cannot be stronger" } */ ! __atomic_compare_exchange_n (&i, &e, 1, 0, __ATOMIC_SEQ_CST, __ATOMIC_RELEASE); /* { dg-warning "invalid failure memory" } */ ! __atomic_compare_exchange_n (&i, &e, 1, 1, __ATOMIC_SEQ_CST, __ATOMIC_ACQ_REL); /* { dg-warning "invalid failure memory" } */ ! ! __atomic_load_n (&i, __ATOMIC_RELEASE); /* { dg-warning "invalid memory model" } */ ! __atomic_load_n (&i, __ATOMIC_ACQ_REL); /* { dg-warning "invalid memory model" } */ ! ! __atomic_store_n (&i, 1, __ATOMIC_ACQUIRE); /* { dg-warning "invalid memory model" } */ ! __atomic_store_n (&i, 1, __ATOMIC_CONSUME); /* { dg-warning "invalid memory model" } */ ! __atomic_store_n (&i, 1, __ATOMIC_ACQ_REL); /* { dg-warning "invalid memory model" } */ i = __atomic_always_lock_free (s, NULL); /* { dg-error "non-constant argument" } */ __atomic_load_n (&i, 44); /* { dg-warning "invalid memory model" } */ ! __atomic_clear (&x, __ATOMIC_ACQUIRE); /* { dg-warning "invalid memory model" } */ ! __atomic_clear (&x, __ATOMIC_ACQ_REL); /* { dg-warning "invalid memory model" } */ }