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

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

(Sorry, repost - I'd meant to cc Mike and Rainer into the
conversation, but forgot to
add them).

On 3 October 2011 13:53, David Gilbert <david.gilbert@linaro.org> wrote:
> 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
>