On 26/05/15 10:32, James Greenhalgh wrote:
Please tie this to the PR which was open in the ChangLog entry.
(aarch64_split_atomic_op): Check for __sync memory models, emit
appropriate initial and final barriers.
I don't see any new initial barriers. I think you are referring to
relaxing the ldaxr to an ldxr for __sync primitives, in which case, say
that.
+/* Emit a post-operation barrier. */
This comment could do with some more detail. What is a post-operation
barrier? When do we need one? What is the MODEL parameter?
+ /* A __sync operation will emit a final fence to stop code hoisting, so the
Can we pick a consistent terminology between fence/barrier? They are
currently used interchangeably, but I think we generally prefer barrier
in the AArch64 port.
- aarch64_emit_load_exclusive (mode, old_out, mem, model_rtx);
+ aarch64_emit_load_exclusive (mode, old_out, mem, load_model_rtx);
To my mind, these two hunks would be marginally easier to follow if
we combined them, as so:
Attached updated patch:
- Expanded the comment for aarch64_emit_post_barrier.
- Used 'barrier' rather than 'fence' in comments.
- Simplified the code for the initial load.
Tested with check-gcc for aarch64-none-linux-gnu.
Ok?
Matthew
2015-06-01 Matthew Wahab <[email protected]>
PR target/65697
* config/aarch64/aarch64.c (aarch64_split_compare_and_swap): Check
for __sync memory models, emit initial loads and final barriers as
appropriate.
From 02effa4c1e3e219f727c88091ebd9938d90c3f8a Mon Sep 17 00:00:00 2001
From: Matthew Wahab <[email protected]>
Date: Fri, 15 May 2015 09:26:28 +0100
Subject: [PATCH 1/3] [AArch64] Strengthen barriers for sync-fetch-op builtin.
Change-Id: I3342a572d672163ffc703e4e51603744680334fc
---
gcc/config/aarch64/aarch64.c | 31 ++++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 083b9b4..b083e12 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -9409,6 +9409,23 @@ aarch64_expand_compare_and_swap (rtx operands[])
emit_insn (gen_rtx_SET (bval, x));
}
+/* Emit a barrier, that is appropriate for memory model MODEL, at the end of a
+ sequence implementing an atomic operation. */
+
+static void
+aarch64_emit_post_barrier (enum memmodel model)
+{
+ const enum memmodel base_model = memmodel_base (model);
+
+ if (is_mm_sync (model)
+ && (base_model == MEMMODEL_ACQUIRE
+ || base_model == MEMMODEL_ACQ_REL
+ || base_model == MEMMODEL_SEQ_CST))
+ {
+ emit_insn (gen_mem_thread_fence (GEN_INT (MEMMODEL_SEQ_CST)));
+ }
+}
+
/* Split a compare and swap pattern. */
void
@@ -9471,6 +9488,8 @@ aarch64_split_atomic_op (enum rtx_code code, rtx old_out, rtx new_out, rtx mem,
{
machine_mode mode = GET_MODE (mem);
machine_mode wmode = (mode == DImode ? DImode : SImode);
+ const enum memmodel model = memmodel_from_int (INTVAL (model_rtx));
+ const bool is_sync = is_mm_sync (model);
rtx_code_label *label;
rtx x;
@@ -9485,7 +9504,13 @@ aarch64_split_atomic_op (enum rtx_code code, rtx old_out, rtx new_out, rtx mem,
old_out = new_out;
value = simplify_gen_subreg (wmode, value, mode, 0);
- aarch64_emit_load_exclusive (mode, old_out, mem, model_rtx);
+ /* The initial load can be relaxed for a __sync operation since a final
+ barrier will be emitted to stop code hoisting. */
+ if (is_sync)
+ aarch64_emit_load_exclusive (mode, old_out, mem,
+ GEN_INT (MEMMODEL_RELAXED));
+ else
+ aarch64_emit_load_exclusive (mode, old_out, mem, model_rtx);
switch (code)
{
@@ -9521,6 +9546,10 @@ aarch64_split_atomic_op (enum rtx_code code, rtx old_out, rtx new_out, rtx mem,
x = gen_rtx_IF_THEN_ELSE (VOIDmode, x,
gen_rtx_LABEL_REF (Pmode, label), pc_rtx);
aarch64_emit_unlikely_jump (gen_rtx_SET (pc_rtx, x));
+
+ /* Emit any final barrier needed for a __sync operation. */
+ if (is_sync)
+ aarch64_emit_post_barrier (model);
}
static void
--
1.9.1
