Re: [Patch 1/4] ARM 64 bit sync atomic operations [V2]

[David Gilbert <david dot gilbert at linaro dot org>]

On 30 September 2011 14:21, Ramana Radhakrishnan
<ramana.radhakrishnan@linaro.org> 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. ?Thanks.

Oops - sorry about those; I'll run it through the check script and nail them.

>>@@ -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

OK.

>>+ ? ? ?/* 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.

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 infrastructure
> 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 flags
   3) Build and check assembler doesn't use ldrexd - compiled with armv5 flags

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 { } {
>>+ ? ?return [check_runtime sync_longlong_runtime {
>>+ ? ? ?#include <stdlib.h>
>>+ ? ? ?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 ?

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