From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32269 invoked by alias); 30 Sep 2011 13:21:57 -0000 Received: (qmail 32240 invoked by uid 22791); 30 Sep 2011 13:21:52 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-pz0-f47.google.com (HELO mail-pz0-f47.google.com) (209.85.210.47) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 30 Sep 2011 13:21:38 +0000 Received: by pzk4 with SMTP id 4so4535814pzk.6 for ; Fri, 30 Sep 2011 06:21:38 -0700 (PDT) MIME-Version: 1.0 Received: by 10.68.31.4 with SMTP id w4mr29987485pbh.20.1317388897849; Fri, 30 Sep 2011 06:21:37 -0700 (PDT) Received: by 10.68.55.101 with HTTP; Fri, 30 Sep 2011 06:21:37 -0700 (PDT) In-Reply-To: <20110726090039.GB6925@davesworkthinkpad> References: <20110701155254.GA5242@davesworkthinkpad> <20110726085910.GA6925@davesworkthinkpad> <20110726090039.GB6925@davesworkthinkpad> Date: Fri, 30 Sep 2011 14:25:00 -0000 Message-ID: Subject: Re: [Patch 1/4] ARM 64 bit sync atomic operations [V2] From: Ramana Radhakrishnan To: "Dr. David Alan Gilbert" Cc: gcc-patches@gcc.gnu.org, rth@redhat.com, joseph@codesourcery.com, patches@linaro.org Content-Type: text/plain; charset=ISO-8859-1 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-09/txt/msg02035.txt.bz2 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. Thanks. >@@ -23590,82 +23637,142 @@ arm_output_sync_loop (emit_f emit, > >+ else >+ { >+ /* Silence false potentially unused warning */ >+ required_value_lo = NULL; >+ required_value_hi = NULL; >+ } > s/NULL/NULL_RTX in a number of places in arm.c >@@ -23516,14 +23530,41 @@ arm_output_strex (emit_f emit, > rtx value, > rtx memory) > { >- const char *suffix = arm_ldrex_suffix (mode); >- rtx operands[3]; >+ rtx operands[4]; > > operands[0] = result; > operands[1] = value; >- operands[2] = memory; >- arm_output_asm_insn (emit, 0, operands, "strex%s%s\t%%0, %%1, %%C2", suffix, >- cc); >+ if (mode != DImode) >+ { >+ const char *suffix = arm_ldrex_suffix (mode); >+ operands[2] = memory; >+ arm_output_asm_insn (emit, 0, operands, "strex%s%s\t%%0, %%1, %%C2", >+ suffix, cc); >+ } >+ else >+ { >+ /* The restrictions on target registers in ARM mode are that the two >+ registers are consecutive and the first one is even; Thumb is >+ actually more flexible, but DI should give us this anyway. >+ Note that the 1st register always gets the lowest word in memory. */ >+ gcc_assert ((REGNO (value) & 1) == 0); >+ operands[2] = gen_rtx_REG (SImode, REGNO (value) + 1); >+ operands[3] = memory; >+ arm_output_asm_insn (emit, 0, operands, "strexd%s\t%%0, %%1, %%2, %%C3", >+ cc); >+ } > The restriction is actually mandatory for ARM state only and thus I'm fine with this assertion being true only in ARM state. 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 infrastructure bits as well but I have a few comments below . >> +# 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 { } { >+ return [check_runtime sync_longlong_runtime { >+ #include >+ int main() >+ { >+ long long l1; >+ >+ if (sizeof(long long)!=8) Space between ')' and ! as well as '=' and 8 >+ exit(1); >+ >+ #ifdef __arm__ Why is this checking only for ARM state ? We could have ldrexd in T2 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 ? cheers Ramana