(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.gilb...@linaro.org> wrote:
> On 30 September 2011 14:21, Ramana Radhakrishnan
> <ramana.radhakrish...@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
>
