From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11622 invoked by alias); 6 Aug 2012 18:51:03 -0000 Received: (qmail 11613 invoked by uid 22791); 6 Aug 2012 18:51:01 -0000 X-SWARE-Spam-Status: No, hits=-6.9 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,TW_LP,TW_LR,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; Mon, 06 Aug 2012 18:50:44 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q76Iog0w020133 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 6 Aug 2012 14:50:42 -0400 Received: from anchor.twiddle.home (vpn-9-86.rdu.redhat.com [10.11.9.86]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q76Ioewk029381; Mon, 6 Aug 2012 14:50:41 -0400 Message-ID: <50201200.3080000@redhat.com> Date: Mon, 06 Aug 2012 18:51:00 -0000 From: Richard Henderson User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120717 Thunderbird/14.0 MIME-Version: 1.0 To: Ulrich Weigand CC: gcc-patches@gcc.gnu.org Subject: Re: [PATCH 2/2] s390: Convert from sync to atomic optabs References: <201208061834.q76IY8HS013445@d06av02.portsmouth.uk.ibm.com> In-Reply-To: <201208061834.q76IY8HS013445@d06av02.portsmouth.uk.ibm.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2012-08/txt/msg00316.txt.bz2 On 08/06/2012 11:34 AM, Ulrich Weigand wrote: > Richard Henderson wrote: > > > Some more comments on this patch. > >> +; Different from movdi_31 in that we have no splitters. >> +(define_insn "atomic_loaddi_1" >> + [(set (match_operand:DI 0 "register_operand" "=d,d,!*f,!*f") >> + (unspec:DI [(match_operand:DI 1 "s_operand" "Q,S,Q,m")] > > The constraint line doesn't look right. ld(y) accept base+index, > so they should get R and T constraints, respectively. Yes, but since I'd limited to s_operand, R seemed weird. > In fact, I wouldn't use "s_operand" as a predicate, since this > excludes valid addresses for those cases, and since in general > I've found it to be preferable to have reload fix up addresses. > So I'd just use "memory_operand" here, and actually in all the > other patterns as well ... (This also simplifes the expander.) In the first patch I did, I had memory_operand and QS, but that ran into reload failures. I assumed I'd just made a mistake. I'll see if I can replicate this for your debugging enjoyment... > "m" is wrong here (and basically everywhere), since it includes > SYMBOL_REFs for lrl etc. on z10. For lpq we need "RT". Ah right. > LPQ and STPQ probably should get a "type" of "other". This may not > model the pipeline exactly, but it's better than just assuming > "integer" by default. These instructions are special (and slow). Noted. > There is one particular inefficiency I have noticed. This code: > > if (!__atomic_compare_exchange_n (&v, &expected, max, 0 , 0, 0)) > abort (); > > from atomic-compare-exchange-3.c gets compiled into: > > l %r3,0(%r2) > larl %r1,v > cs %r3,%r4,0(%r1) > ipm %r1 > sra %r1,28 > st %r3,0(%r2) > ltr %r1,%r1 > jne .L3 > > which is extremely inefficient; it converts the condition code into > an integer using the slow ipm, sra sequence, just so that it can > convert the integer back into a condition code via ltr and branch > on it ... ... > Is there a way of structuring the atomic optabs/expander so that the store > gets done either before or after all the CC operations? No. But I'm a bit confused since a store doesn't affect the flags, so I'm not sure why the EQ can't be combined with the branch. I'll give that another look. All that said, one of the things that MacLeod's gimple_atomic will achieve is allowing two register outputs, which (for the common case) will eliminate the store entirely. r~