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
