diff mbox

[ARM] Add atomic_loaddi pattern

Message ID 4F9B0FEF.50406@redhat.com
State New
Headers show

Commit Message

Richard Henderson April 27, 2012, 9:30 p.m. UTC
We can perform a single-copy atomic load with an ldrexd insn.
If the load is all we care about, we need not pair this with
a strexd.

Ok?


r~
* config/arm/arm.md (UNSPEC_LL): New.
	* config/arm/sync.md (atomic_loaddi, atomic_loaddi_1): New.

Comments

Richard Earnshaw April 30, 2012, 8:47 a.m. UTC | #1
On 27/04/12 22:30, Richard Henderson wrote:
> We can perform a single-copy atomic load with an ldrexd insn.
> If the load is all we care about, we need not pair this with
> a strexd.
> 
> Ok?
> 
> 
> r~
> 
> 
> d-arm-ldi
> 
> 
> 	* config/arm/arm.md (UNSPEC_LL): New.
> 	* config/arm/sync.md (atomic_loaddi, atomic_loaddi_1): New.
> 

> diff --git a/gcc/config/arm/sync.md b/gcc/config/arm/sync.md
> index 03838f5..de2da3b 100644
> --- a/gcc/config/arm/sync.md
> +++ b/gcc/config/arm/sync.md


> +    operands[2] = gen_rtx_REG (SImode, REGNO (target) + 1);
> +    return "ldrexd%?\t%0, %2, %C1";
> +  }

Use "ldrexd%?\t%0, %H0, %C1", then you don't need to construct operands[2]

Otherwise, OK.

R.
Andrew Haley April 30, 2012, 8:51 a.m. UTC | #2
On 04/27/2012 10:30 PM, Richard Henderson wrote:
> We can perform a single-copy atomic load with an ldrexd insn.
> If the load is all we care about, we need not pair this with
> a strexd.

Can we?  It's good to know.  I have had a long email exchange with
engineers at ARM, and they would not say that this was safe.  If they
have changed their mind, I'd like to see chapter and verse.

Andrew.
Richard Earnshaw April 30, 2012, 10:50 a.m. UTC | #3
On 30/04/12 09:51, Andrew Haley wrote:
> On 04/27/2012 10:30 PM, Richard Henderson wrote:
>> We can perform a single-copy atomic load with an ldrexd insn.
>> If the load is all we care about, we need not pair this with
>> a strexd.
> 
> Can we?  It's good to know.  I have had a long email exchange with
> engineers at ARM, and they would not say that this was safe.  If they
> have changed their mind, I'd like to see chapter and verse.
> 
> Andrew.
> 

The ARM ARM lists a number of single-copy atomic operations.  For v7,
the list includes:

- memory accesses caused by LDREXD and STREXD instructions to
doubleword-aligned locations.

Of course, there is a potential performance issue from heavy use of the
global monitor to ensure coherency.

R.
Andrew Haley April 30, 2012, 11:23 a.m. UTC | #4
On 04/30/2012 11:50 AM, Richard Earnshaw wrote:
> On 30/04/12 09:51, Andrew Haley wrote:
>> On 04/27/2012 10:30 PM, Richard Henderson wrote:
>>> We can perform a single-copy atomic load with an ldrexd insn.
>>> If the load is all we care about, we need not pair this with
>>> a strexd.
>>
>> Can we?  It's good to know.  I have had a long email exchange with
>> engineers at ARM, and they would not say that this was safe.  If they
>> have changed their mind, I'd like to see chapter and verse.
> 
> The ARM ARM lists a number of single-copy atomic operations.  For v7,
> the list includes:
> 
> - memory accesses caused by LDREXD and STREXD instructions to
> doubleword-aligned locations.
>
> Of course, there is a potential performance issue from heavy use of the
> global monitor to ensure coherency.

Aha!  That is excellent news. that none of us managed to discover.
I shall immediately change the code we use in OpenJDK.

Thanks,
Andrew.
Richard Henderson April 30, 2012, 5:01 p.m. UTC | #5
On 04/30/2012 01:47 AM, Richard Earnshaw wrote:
>
>> >  +    operands[2] = gen_rtx_REG (SImode, REGNO (target) + 1);
>> >  +    return "ldrexd%?\t%0, %2, %C1";
>> >  +  }
> Use "ldrexd%?\t%0, %H0, %C1", then you don't need to construct operands[2]
>
> Otherwise, OK.

Ah, nice.  I also fixed the other instance of ldrexd to use %H.


r~
diff mbox

Patch

diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 79eff0e..75d2565 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -117,6 +117,7 @@ 
 			; that.
   UNSPEC_UNALIGNED_STORE ; Same for str/strh.
   UNSPEC_PIC_UNIFIED    ; Create a common pic addressing form.
+  UNSPEC_LL		; Represent an unpaired load-register-exclusive.
 ])
 
 ;; UNSPEC_VOLATILE Usage:
diff --git a/gcc/config/arm/sync.md b/gcc/config/arm/sync.md
index 03838f5..de2da3b 100644
--- a/gcc/config/arm/sync.md
+++ b/gcc/config/arm/sync.md
@@ -65,6 +65,40 @@ 
    (set_attr "conds" "unconditional")
    (set_attr "predicable" "no")])
 
+;; Note that ldrd and vldr are *not* guaranteed to be single-copy atomic,
+;; even for a 64-bit aligned address.  Instead we use a ldrexd unparied
+;; with a store.
+(define_expand "atomic_loaddi"
+  [(match_operand:DI 0 "s_register_operand")		;; val out
+   (match_operand:DI 1 "mem_noofs_operand")		;; memory
+   (match_operand:SI 2 "const_int_operand")]		;; model
+  "TARGET_HAVE_LDREXD && ARM_DOUBLEWORD_ALIGN"
+{
+  enum memmodel model = (enum memmodel) INTVAL (operands[2]);
+  expand_mem_thread_fence (model);
+  emit_insn (gen_atomic_loaddi_1 (operands[0], operands[1]));
+  if (model == MEMMODEL_SEQ_CST)
+    expand_mem_thread_fence (model);
+  DONE;
+})
+
+(define_insn "atomic_loaddi_1"
+  [(set (match_operand:DI 0 "s_register_operand" "=r")
+	(unspec:DI [(match_operand:DI 1 "mem_noofs_operand" "Ua")]
+		   UNSPEC_LL))]
+  "TARGET_HAVE_LDREXD && ARM_DOUBLEWORD_ALIGN"
+  {
+    rtx target = operands[0];
+    /* 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 (target) & 1) == 0);
+    operands[2] = gen_rtx_REG (SImode, REGNO (target) + 1);
+    return "ldrexd%?\t%0, %2, %C1";
+  }
+  [(set_attr "predicable" "yes")])
+
 (define_expand "atomic_compare_and_swap<mode>"
   [(match_operand:SI 0 "s_register_operand" "")		;; bool out
    (match_operand:QHSD 1 "s_register_operand" "")	;; val out