diff mbox

Fix several atomic_test_and_set problems

Message ID 4F1F0D08.4080507@redhat.com
State New
Headers show

Commit Message

Richard Henderson Jan. 24, 2012, 7:56 p.m. UTC
I thought there was a PR for this, but I can't find it.

A report about problems using the atomic_test_and_set pattern for SH
showed that we weren't being completely sane with the mode handling
of the boolean output.  Further investigation showed that the docs
didn't match the implementation (or vice-versa), and that the Sparc
implementation didn't match either (to be fixed in another patch).

The need to store an arbitrary non-zero value in the byte means that
the documentation that we take a bool* can't be strictly correct.
I thought about changing it to char* or making it a fully variable
type, as with the other atomic builtins.  But in the end I left the
existing implementation using void*.  As long as the user is using
any integral type we'll still get an (endian dependent) arbitrary
non-zero result.  Which should be fine.

Fortunately, all of the targets that need to use atomic_test_and_set
(because they don't have atomic_exchange or better) of which I am
aware use a test-and-set insn that is byte based.  Which means that
so far we don't need to receive more information from the builtin
about space available, or handle any mode besides QImode when
expanding the operation.

Irritatingly, I made a mistake while moving this patch around and
committed a slightly broken version.  Thus what should have been
one commit is split up into the two patches below.

Bootstrapped on powerpc64 (where we merely check for Werror's),
and sparc64 (where we actually make use of ldstub).

Committed.


r~


PS. I'd been hoping to be able to use bts on x86, so that we have a
major platform using this code, but I'd forgotten there's no byte
variant of that instruction.  Ah well.
* optabs.c (CODE_FOR_atomic_test_and_set): Provide default.
	(maybe_emit_atomic_test_and_set): New.
	(expand_sync_lock_test_and_set): Use it.
	(expand_atomic_test_and_set): Likewise.
	* doc/extend.texi (__atomic_test_and_set): Adjust the docs to match
	the implementation; clarify implementation defined details.
	* doc/md.texi (atomic_test_and_set): Document.
* optabs.c (maybe_emit_atomic_test_and_set): Mark model unused.
        Allow non-QImode mem inputs.


diff --git a/gcc/optabs.c b/gcc/optabs.c
index 0f6d763..ce569d6 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -1,7 +1,7 @@
 /* Expand the basic unary and binary arithmetic operations, for GNU compiler.
    Copyright (C) 1987, 1988, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
    1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010,
-   2011  Free Software Foundation, Inc.
+   2011, 2012  Free Software Foundation, Inc.
 
 This file is part of GCC.
 
@@ -7315,7 +7315,8 @@ maybe_emit_compare_and_swap_exchange_loop (rtx target, rtx mem, rtx val)
 #endif
 
 static rtx
-maybe_emit_atomic_test_and_set (rtx target, rtx mem, enum memmodel model)
+maybe_emit_atomic_test_and_set (rtx target, rtx mem,
+				enum memmodel model ATTRIBUTE_UNUSED)
 {
   enum machine_mode pat_bool_mode;
   const struct insn_data_d *id;
@@ -7329,7 +7330,13 @@ maybe_emit_atomic_test_and_set (rtx target, rtx mem, enum memmodel model)
   /* ??? We only support test-and-set on single bytes at the moment.
      We'd have to change the builtin to allow wider memories.  */
   gcc_checking_assert (id->operand[1].mode == QImode);
-  gcc_checking_assert (GET_MODE (mem) == QImode);
+
+  /* While we always get QImode from __atomic_test_and_set, we get
+     other memory modes from __sync_lock_test_and_set.  Note that we
+     use no endian adjustment here.  This matches the 4.6 behavior
+     in the Sparc backend.  */
+  if (GET_MODE (mem) != QImode)
+    mem = adjust_address_nv (mem, QImode, 0);
 
   if (target == NULL || GET_MODE (target) != pat_bool_mode)
     target = gen_reg_rtx (pat_bool_mode);
diff mbox

Patch

diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 229e87c..e159736 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -7202,11 +7202,12 @@  All memory models are valid.
 
 @end deftypefn
 
-@deftypefn {Built-in Function} bool __atomic_test_and_set (bool *ptr, int memmodel)
+@deftypefn {Built-in Function} bool __atomic_test_and_set (void *ptr, int memmodel)
 
 This built-in function performs an atomic test-and-set operation on
-@code{*@var{ptr}}.  @code{*@var{ptr}} is set to the value 1 and
-the previous contents are returned.
+the byte at @code{*@var{ptr}}.  The byte is set to some implementation
+defined non-zero "set" value and the return value is @code{true} if and only
+if the previous contents were "set".
 
 All memory models are valid.
 
diff --git a/gcc/doc/md.texi b/gcc/doc/md.texi
index bcfd420..afdc3e3 100644
--- a/gcc/doc/md.texi
+++ b/gcc/doc/md.texi
@@ -5893,6 +5893,19 @@  the operation followed by the arithmetic operation required to produce the
 result.  If none of these are available a compare-and-swap loop will be
 used.
 
+@cindex @code{atomic_test_and_set} instruction pattern
+@item @samp{atomic_test_and_set}
+
+This pattern emits code for @code{__builtin_atomic_test_and_set}.
+Operand 0 is an output operand which is set to true if the previous
+previous contents of the byte was "set", and false otherwise.  Operand 1
+is the @code{QImode} memory to be modified.  Operand 2 is the memory
+model to be used.
+
+The specific value that defines "set" is implementation defined, and
+is normally based on what is performed by the native atomic test and set
+instruction.
+
 @cindex @code{mem_thread_fence@var{mode}} instruction pattern
 @item @samp{mem_thread_fence@var{mode}}
 This pattern emits code required to implement a thread fence with
diff --git a/gcc/optabs.c b/gcc/optabs.c
index a532ba3..0f6d763 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -7304,11 +7304,41 @@  maybe_emit_compare_and_swap_exchange_loop (rtx target, rtx mem, rtx val)
   return NULL_RTX;
 }
 
+/* This function tries to implement an atomic test-and-set operation
+   using the atomic_test_and_set instruction pattern.  A boolean value
+   is returned from the operation, using TARGET if possible.  */
+
 #ifndef HAVE_atomic_test_and_set
 #define HAVE_atomic_test_and_set 0
+#define CODE_FOR_atomic_test_and_set CODE_FOR_nothing
 #define gen_atomic_test_and_set(x,y,z)  (gcc_unreachable (), NULL_RTX)
 #endif
 
+static rtx
+maybe_emit_atomic_test_and_set (rtx target, rtx mem, enum memmodel model)
+{
+  enum machine_mode pat_bool_mode;
+  const struct insn_data_d *id;
+
+  if (!HAVE_atomic_test_and_set)
+    return NULL_RTX;
+
+  id = &insn_data[CODE_FOR_atomic_test_and_set];
+  pat_bool_mode = id->operand[0].mode;
+
+  /* ??? We only support test-and-set on single bytes at the moment.
+     We'd have to change the builtin to allow wider memories.  */
+  gcc_checking_assert (id->operand[1].mode == QImode);
+  gcc_checking_assert (GET_MODE (mem) == QImode);
+
+  if (target == NULL || GET_MODE (target) != pat_bool_mode)
+    target = gen_reg_rtx (pat_bool_mode);
+
+  emit_insn (gen_atomic_test_and_set (target, mem, GEN_INT (model)));
+
+  return target;
+}
+
 /* This function expands the legacy _sync_lock test_and_set operation which is
    generally an atomic exchange.  Some limited targets only allow the
    constant 1 to be stored.  This is an ACQUIRE operation. 
@@ -7323,20 +7353,21 @@  expand_sync_lock_test_and_set (rtx target, rtx mem, rtx val)
 
   /* Try an atomic_exchange first.  */
   ret = maybe_emit_atomic_exchange (target, mem, val, MEMMODEL_ACQUIRE);
+  if (ret)
+    return ret;
 
-  if (!ret)
-    ret = maybe_emit_sync_lock_test_and_set (target, mem, val,
-					     MEMMODEL_ACQUIRE);
-  if (!ret)
-    ret = maybe_emit_compare_and_swap_exchange_loop (target, mem, val);
+  ret = maybe_emit_sync_lock_test_and_set (target, mem, val, MEMMODEL_ACQUIRE);
+  if (ret)
+    return ret;
+
+  ret = maybe_emit_compare_and_swap_exchange_loop (target, mem, val);
+  if (ret)
+    return ret;
 
   /* If there are no other options, try atomic_test_and_set if the value
      being stored is 1.  */
-  if (!ret && val == const1_rtx && HAVE_atomic_test_and_set)
-    {
-      ret = gen_atomic_test_and_set (target, mem, GEN_INT (MEMMODEL_ACQUIRE));
-      emit_insn (ret);
-    }
+  if (val == const1_rtx)
+    ret = maybe_emit_atomic_test_and_set (target, mem, MEMMODEL_ACQUIRE);
 
   return ret;
 }
@@ -7351,28 +7382,26 @@  rtx
 expand_atomic_test_and_set (rtx target, rtx mem, enum memmodel model)
 {
   enum machine_mode mode = GET_MODE (mem);
-  rtx ret = NULL_RTX;
+  rtx ret;
+
+  ret = maybe_emit_atomic_test_and_set (target, mem, model);
+  if (ret)
+    return ret;
 
   if (target == NULL_RTX)
     target = gen_reg_rtx (mode);
 
-  if (HAVE_atomic_test_and_set)
-    {
-      ret = gen_atomic_test_and_set (target, mem, GEN_INT (MEMMODEL_ACQUIRE));
-      emit_insn (ret);
-      return ret;
-    }
-
   /* If there is no test and set, try exchange, then a compare_and_swap loop,
      then __sync_test_and_set.  */
   ret = maybe_emit_atomic_exchange (target, mem, const1_rtx, model);
+  if (ret)
+    return ret;
 
-  if (!ret)
-    ret = maybe_emit_compare_and_swap_exchange_loop (target, mem, const1_rtx);
-
-  if (!ret)
-    ret = maybe_emit_sync_lock_test_and_set (target, mem, const1_rtx, model);
+  ret = maybe_emit_compare_and_swap_exchange_loop (target, mem, const1_rtx);
+  if (ret)
+    return ret;
 
+  ret = maybe_emit_sync_lock_test_and_set (target, mem, const1_rtx, model);
   if (ret)
     return ret;