From patchwork Mon Aug 7 08:42:25 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alexander Monakov X-Patchwork-Id: 798527 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-459919-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="n8icgjRQ"; dkim-atps=neutral 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 3xQrdJ00Kdz9rxm for ; Mon, 7 Aug 2017 18:42:43 +1000 (AEST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:in-reply-to:message-id:references :mime-version:content-type; q=dns; s=default; b=ctGYvru/n/6RoKOV O8DhPf8Ran2/GebPHr6J/4NiGnxMG5Kdfw1Y/YgKRMFc7vw8OQgn3EKGlI2QevCS ng3Bts4uQmJxghjBXRXyhqpsI1d5xtFamJRIVWnENwIN9r2BtvzqwRWlMI/yY96+ HYoGJJDmawupDzyRC8rBRZoj/7k= 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:date :from:to:cc:subject:in-reply-to:message-id:references :mime-version:content-type; s=default; bh=8w456Bm7yfbyI5gEnhSxzM B82zY=; b=n8icgjRQ4Wn2y/hvg4ycDz+6cJqEIJjMheqtbg8InPEqJhoEzIg3z8 dbHLWx8ly4DcXg572hHZVbnjB98gN26IWi/1wuz2CdyjAeoXycOZK8ZuJUS9N25Q buvXbe/WdcJQrULxt2OQAf/SWdDHQYpo3lRXEL3imN8FvK/lJdDSg= Received: (qmail 128221 invoked by alias); 7 Aug 2017 08:42:32 -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 128207 invoked by uid 89); 7 Aug 2017 08:42:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.7 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=Hx-languages-length:4794 X-HELO: smtp.ispras.ru Received: from bran.ispras.ru (HELO smtp.ispras.ru) (83.149.199.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 07 Aug 2017 08:42:28 +0000 Received: from monopod.intra.ispras.ru (monopod.intra.ispras.ru [10.10.3.121]) by smtp.ispras.ru (Postfix) with ESMTP id DBE8160916; Mon, 7 Aug 2017 11:42:25 +0300 (MSK) Date: Mon, 7 Aug 2017 11:42:25 +0300 (MSK) From: Alexander Monakov To: Richard Sandiford cc: gcc-patches@gcc.gnu.org, law@redhat.com Subject: Re: [PATCH 1/3] optabs: ensure mem_thread_fence is a compiler barrier In-Reply-To: <877eyiidxr.fsf@linaro.org> Message-ID: References: <20170802174548.12344-1-amonakov@ispras.ru> <877eyiidxr.fsf@linaro.org> User-Agent: Alpine 2.20.13 (LNX 116 2015-12-14) MIME-Version: 1.0 On Sat, 5 Aug 2017, Richard Sandiford wrote: > It would be simpler to test whether targetm.gen_mem_thread_fence > returns NULL. > > This feels a bit hacky though. Checking whether a generator produced no > instructions is usually the test for whether the generator FAILed, which > should normally be treated as though the pattern wasn't defined to start > with. This is useful if the FAIL condition is too complex to put in the > define_expand/insn C condition. > > In those contexts, if a generator wants to succeed and generate no > instructions, it should emit a note instead. (Yes, it would be good > to have a nicer interface...) > > So IMO it's confusing to overload the empty sequence to mean something > else in this context, and it wouldn't work if a target does emit the kind > of "I didn't FAIL" note just mentioned. Thanks for the elucidation, I appreciate it. > FWIW, I agree with Jeff in the previous thread that (compared to this) > it would be more robust to make optabs.c emit the compile barrier > unconditionally and just leave the target to emit a machine barrier. OK, I've changed the patch accordingly. I've also factored out the is_mm_relaxed check in this iteration to simplify resulting code. PR target/80640 * doc/md.texi (mem_thread_fence): Remove mention of mode. Rewrite. * optabs.c (expand_mem_thread_fence): Emit a compiler barrier when using targetm.gen_mem_thread_fence. testsuite/ * gcc.dg/atomic/pr80640.c: New testcase. diff --git a/gcc/doc/md.texi b/gcc/doc/md.texi index ea959208c98..64a137ecad0 100644 --- a/gcc/doc/md.texi +++ b/gcc/doc/md.texi @@ -7044,14 +7044,20 @@ If these patterns are not defined, attempts will be made to use counterparts. If none of these are available a compare-and-swap loop will be used. -@cindex @code{mem_thread_fence@var{mode}} instruction pattern -@item @samp{mem_thread_fence@var{mode}} +@cindex @code{mem_thread_fence} instruction pattern +@item @samp{mem_thread_fence} This pattern emits code required to implement a thread fence with memory model semantics. Operand 0 is the memory model to be used. -If this pattern is not specified, all memory models except -@code{__ATOMIC_RELAXED} will result in issuing a @code{sync_synchronize} -barrier pattern. +For the @code{__ATOMIC_RELAXED} model no instructions need to be issued +and this expansion is not invoked. + +The compiler always emits a compiler memory barrier regardless of what +expanding this pattern produced. + +If this pattern is not defined, the compiler falls back to expanding the +@code{memory_barrier} pattern, then to emitting @code{__sync_synchronize} +library call, and finally to just placing a compiler memory barrier. @cindex @code{mem_signal_fence@var{mode}} instruction pattern @item @samp{mem_signal_fence@var{mode}} diff --git a/gcc/optabs.c b/gcc/optabs.c index a9900657a58..71b74dd5feb 100644 --- a/gcc/optabs.c +++ b/gcc/optabs.c @@ -6297,17 +6297,19 @@ expand_asm_memory_barrier (void) void expand_mem_thread_fence (enum memmodel model) { + if (is_mm_relaxed (model)) + return; if (targetm.have_mem_thread_fence ()) - emit_insn (targetm.gen_mem_thread_fence (GEN_INT (model))); - else if (!is_mm_relaxed (model)) { - if (targetm.have_memory_barrier ()) - emit_insn (targetm.gen_memory_barrier ()); - else if (synchronize_libfunc != NULL_RTX) - emit_library_call (synchronize_libfunc, LCT_NORMAL, VOIDmode, 0); - else - expand_asm_memory_barrier (); + emit_insn (targetm.gen_mem_thread_fence (GEN_INT (model))); + expand_asm_memory_barrier (); } + else if (targetm.have_memory_barrier ()) + emit_insn (targetm.gen_memory_barrier ()); + else if (synchronize_libfunc != NULL_RTX) + emit_library_call (synchronize_libfunc, LCT_NORMAL, VOIDmode, 0); + else + expand_asm_memory_barrier (); } /* This routine will either emit the mem_signal_fence pattern or issue a diff --git a/gcc/testsuite/gcc.dg/atomic/pr80640.c b/gcc/testsuite/gcc.dg/atomic/pr80640.c new file mode 100644 index 00000000000..fd17978a482 --- /dev/null +++ b/gcc/testsuite/gcc.dg/atomic/pr80640.c @@ -0,0 +1,34 @@ +/* { dg-do run } */ +/* { dg-options "-pthread" } */ +/* { dg-require-effective-target pthread } */ + +#include + +static volatile int sem1; +static volatile int sem2; + +static void *f(void *va) +{ + void **p = va; + if (*p) return *p; + sem1 = 1; + while (!sem2); + __atomic_thread_fence(__ATOMIC_ACQUIRE); + // GCC used to RTL-CSE this and the first load, causing 0 to be returned + return *p; +} + +int main() +{ + void *p = 0; + pthread_t thr; + if (pthread_create(&thr, 0, f, &p)) + return 2; + while (!sem1); + __atomic_thread_fence(__ATOMIC_ACQUIRE); + p = &p; + __atomic_thread_fence(__ATOMIC_RELEASE); + sem2 = 1; + pthread_join(thr, &p); + return !p; +}