From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14208 invoked by alias); 3 Oct 2011 12:53:29 -0000 Received: (qmail 14063 invoked by uid 22791); 3 Oct 2011 12:53:28 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-gy0-f175.google.com (HELO mail-gy0-f175.google.com) (209.85.160.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 03 Oct 2011 12:53:09 +0000 Received: by gyg8 with SMTP id 8so3995088gyg.20 for ; Mon, 03 Oct 2011 05:53:09 -0700 (PDT) MIME-Version: 1.0 Received: by 10.223.54.202 with SMTP id r10mr8818473fag.114.1317646388774; Mon, 03 Oct 2011 05:53:08 -0700 (PDT) Received: by 10.152.41.2 with HTTP; Mon, 3 Oct 2011 05:53:08 -0700 (PDT) In-Reply-To: References: <20110701155254.GA5242@davesworkthinkpad> <20110726085910.GA6925@davesworkthinkpad> <20110726090039.GB6925@davesworkthinkpad> Date: Mon, 03 Oct 2011 12:53:00 -0000 Message-ID: Subject: Re: [Patch 1/4] ARM 64 bit sync atomic operations [V2] From: David Gilbert To: Ramana Radhakrishnan Cc: gcc-patches@gcc.gnu.org, rth@redhat.com, joseph@codesourcery.com, patches@linaro.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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: 2011-10/txt/msg00079.txt.bz2 On 30 September 2011 14:21, Ramana Radhakrishnan wrote: > Hi Dave, > > > The nit-picky bit - There are still a number of formatting issues with > your patch . Could you run your patch through > contrib/check_GNU_style.sh and correct these. These are typically > around problems with the number of spaces between a full stop and the > end of comment, lines with trailing whitespaces and a few lines with > number of characters > 80. =A0Thanks. Oops - sorry about those; I'll run it through the check script and nail the= m. >>@@ -23590,82 +23637,142 @@ arm_output_sync_loop (emit_f emit, >> >>+ =A0 =A0 =A0else >>+ =A0 =A0 =A0{ >>+ =A0 =A0 =A0 =A0/* Silence false potentially unused warning */ >>+ =A0 =A0 =A0 =A0required_value_lo =3D NULL; >>+ =A0 =A0 =A0 =A0required_value_hi =3D NULL; >>+ =A0 =A0 =A0} >> > > s/NULL/NULL_RTX in a number of places in arm.c OK. >>+ =A0 =A0 =A0/* The restrictions on target registers in ARM mode are that= the two >>+ =A0 =A0 =A0 registers are consecutive and the first one is even; Thumb = is >>+ =A0 =A0 =A0 actually more flexible, but DI should give us this anyway. >>+ =A0 =A0 =A0 Note that the 1st register always gets the lowest word in m= emory. =A0*/ >>+ =A0 =A0 =A0gcc_assert ((REGNO (value) & 1) =3D=3D 0); >>+ =A0 =A0 =A0operands[2] =3D gen_rtx_REG (SImode, REGNO (value) + 1); >>+ =A0 =A0 =A0operands[3] =3D memory; >>+ =A0 =A0 =A0arm_output_asm_insn (emit, 0, operands, "strexd%s\t%%0, %%1,= %%2, %%C3", >>+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 cc); >>+ =A0 =A0} >> > > The restriction is actually mandatory for ARM state only and thus I'm fine > with this assertion being true only in ARM state. OK, I can make the assert only for thumb mode; but I thought the simpler logic was better and should hold true anyway because of DI mode allocation. > I don't like duplicating the tests from gcc.dg into gcc.target/arm. > If you wanted to check for assembler output specific to a target you could > add your own conditions to the test in gcc.dg and conditionalize that on > target arm_eabi > > Something like : > > { dg-final { scan-assembler "ldrexd\t"} {target arm_eabi}} } . > > I would like a testsuite maintainer to comment on the testsuite infrastru= cture > bits as well but I have a few comments below . As discussed, I don't like the dupes either - the problem is that we have 3 tests with identical code but different dg annotation: 1) Build & run and check that the sync behaves correctly - using whatever compile flags you happen to have. (gcc.dg version) 2) Build and check assembler for use of ldrexd - compiled with armv6k fl= ags 3) Build and check assembler doesn't use ldrexd - compiled with armv5 fl= ags Because (2) and (3) include different dg-add-options lines I don't see how I can combine them. The suggestion that I'm OK with is to #include the gcc.dg one in the gcc.arm one. >>> +# Return 1 if the target supports atomic operations on "long long" and= can actually >>+# execute them >>+# So far only put checks in for ARM, others may want to add their own >>+proc check_effective_target_sync_longlong { } { >>+ =A0 =A0return [check_runtime sync_longlong_runtime { >>+ =A0 =A0 =A0#include >>+ =A0 =A0 =A0int main() >>+ =A0 =A0 =A0{ >>+ =A0 =A0 =A0long long l1; >>+ >>+ =A0 =A0 =A0if (sizeof(long long)!=3D8) > > Space between ')' and ! as well as '=3D' and 8 > >>+ =A0 =A0 =A0 =A0exit(1); >>+ >>+ =A0 =A0 =A0#ifdef __arm__ > > Why is this checking only for ARM state ? We could have ldrexd in T2 as > well ? Because __arm__ gets defined for either thumb or arm mode; in thumb mode we just get __thumb__ (and __thumb2__) defined as well. > Otherwise the functionality looks good to me. Can you confirm that > this has survived a testrun for v7-a thumb2 and v7-a arm state ? Yes it did. I'll give it another whirl later today after I go and fix the formatting niggles and mvoe the test. Thanks for the review. Dave