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

[Ramana Radhakrishnan <ramana dot radhakrishnan at linaro dot org>]

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

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