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

2011-10-03 Thread David Gilbert
(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  wrote:
> On 30 September 2011 14:21, Ramana Radhakrishnan
>  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 
>>>+      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
>


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

2011-10-03 Thread David Gilbert
On 30 September 2011 14:21, Ramana Radhakrishnan
 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 
>>+      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


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

2011-09-30 Thread Ramana Radhakrishnan
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