[m68k, sparc] Fix atomic_test_and_set

[m68k, sparc] Fix atomic_test_and_set

[Richard Henderson]

[-- Attachment #1: Type: text/plain, Size: 349 bytes --]

Two of the patches have been posted here before; the libstdc++
patch was approved by Benjamin.

All of the patches tested on sparc64-linux, and sanity checked
on x86_64-linux.  I've cross-compiled for m68k-linux, but I've
only been able to visually sanity check the code in libstdc++.

Committed.  Hopefully that wraps up the atomic patches...


r~

[-- Attachment #2: z1 --]
[-- Type: text/plain, Size: 2133 bytes --]

	* optabs.c (gen_atomic_test_and_set): Remove default.
	(maybe_emit_atomic_test_and_set): Use maybe_expand_insn.


diff --git a/gcc/optabs.c b/gcc/optabs.c
index fb01320..87cce8e 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -7311,39 +7311,34 @@ maybe_emit_compare_and_swap_exchange_loop (rtx target, rtx mem, rtx val)
 #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 (), (void) (0 && (x) && (y) && (z)), 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;
+  struct expand_operand ops[3];
 
   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);
-
   /* 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.  */
+  gcc_checking_assert
+    (insn_data[CODE_FOR_atomic_test_and_set].operand[1].mode == QImode);
   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);
-
-  emit_insn (gen_atomic_test_and_set (target, mem, GEN_INT (model)));
+  pat_bool_mode = insn_data[CODE_FOR_atomic_test_and_set].operand[0].mode;
+  create_output_operand (&ops[0], target, pat_bool_mode);
+  create_fixed_operand (&ops[1], mem);
+  create_integer_operand (&ops[2], model);
 
-  return target;
+  if (maybe_expand_insn (CODE_FOR_atomic_test_and_set, 3, ops))
+    return ops[0].value;
+  return NULL_RTX;
 }
 
 /* This function expands the legacy _sync_lock test_and_set operation which is

[-- Attachment #3: z2 --]
[-- Type: text/plain, Size: 2667 bytes --]

	* target.def (TARGET_ATOMIC_TEST_AND_SET_TRUEVAL): New.
	* c-cppbuiltin.c (cpp_atomic_builtins): Define
	__GCC_ATOMIC_TEST_AND_SET_TRUEVAL.
	* doc/tm.texi.in (TARGET_ATOMIC_TEST_AND_SET_TRUEVAL): Add doc hook.
	* doc/tm.texi: Rebuild.


diff --git a/gcc/c-family/c-cppbuiltin.c b/gcc/c-family/c-cppbuiltin.c
index 7e7b9c1..608dba6 100644
--- a/gcc/c-family/c-cppbuiltin.c
+++ b/gcc/c-family/c-cppbuiltin.c
@@ -670,6 +670,11 @@ cpp_atomic_builtins (cpp_reader *pfile)
   builtin_define_with_int_value ("__GCC_ATOMIC_LLONG_LOCK_FREE", 
 		(have_swap[SWAP_INDEX (long_long_integer_type_node)]? 2 : 1));
 
+  /* If we're dealing with a "set" value that doesn't exactly correspond
+     to a boolean truth value, let the library work around that.  */
+  builtin_define_with_int_value ("__GCC_ATOMIC_TEST_AND_SET_TRUEVAL",
+				 targetm.atomic_test_and_set_trueval);
+
   /* ptr_type_node can't be used here since ptr_mode is only set when
      toplev calls backend_init which is not done with -E  or pch.  */
   psize = POINTER_SIZE / BITS_PER_UNIT;
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index ceb0d1e..91e4b04 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -11359,3 +11359,7 @@ value of @code{TARGET_CONST_ANCHOR} is a power of 2.  For example, on
 MIPS, where add-immediate takes a 16-bit signed value,
 @code{TARGET_CONST_ANCHOR} is set to @samp{0x8000}.  The default value
 is zero, which disables this optimization.  @end deftypevr
+
+@deftypevr {Target Hook} {unsigned char} TARGET_ATOMIC_TEST_AND_SET_TRUEVAL
+This value should be set if the result written by @code{atomic_test_and_set} is not exactly 1, i.e. the @code{bool} @code{true}.
+@end deftypevr
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 55c8432..0ebc15d 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -11237,3 +11237,5 @@ value of @code{TARGET_CONST_ANCHOR} is a power of 2.  For example, on
 MIPS, where add-immediate takes a 16-bit signed value,
 @code{TARGET_CONST_ANCHOR} is set to @samp{0x8000}.  The default value
 is zero, which disables this optimization.  @end deftypevr
+
+@hook TARGET_ATOMIC_TEST_AND_SET_TRUEVAL
diff --git a/gcc/target.def b/gcc/target.def
index f86f782..6084b21 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -2667,6 +2667,13 @@ DEFHOOK
  enum unwind_info_type, (void),
  default_debug_unwind_info)
 
+DEFHOOKPOD
+(atomic_test_and_set_trueval,
+ "This value should be set if the result written by\
+ @code{atomic_test_and_set} is not exactly 1, i.e. the\
+ @code{bool} @code{true}.",
+ unsigned char, 1)
+ 
 /* Leave the boolean fields at the end.  */
 
 /* True if we can create zeroed data by switching to a BSS section

[-- Attachment #4: z3 --]
[-- Type: text/plain, Size: 1373 bytes --]

        * include/bits/atomic_base.h (__atomic_flag_base): Define _M_i
        based on the value of __GCC_ATOMIC_TEST_AND_SET_TRUEVAL.
        (ATOMIC_FLAG_INIT): Initialize with 0, not false.
        (atomic_flag::atomic_flag): Use __GCC_ATOMIC_TEST_AND_SET_TRUEVAL.


diff --git a/libstdc++-v3/include/bits/atomic_base.h b/libstdc++-v3/include/bits/atomic_base.h
index ef17b7e..aa43bcc 100644
--- a/libstdc++-v3/include/bits/atomic_base.h
+++ b/libstdc++-v3/include/bits/atomic_base.h
@@ -227,12 +227,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   struct __atomic_flag_base
   {
+    /* The target's "set" value for test-and-set may not be exactly 1.  */
+#if __GCC_ATOMIC_TEST_AND_SET_TRUEVAL == 1
     bool _M_i;
+#else
+    unsigned char _M_i;
+#endif
   };
 
   _GLIBCXX_END_EXTERN_C
 
-#define ATOMIC_FLAG_INIT { false }
+#define ATOMIC_FLAG_INIT { 0 }
 
   /// atomic_flag
   struct atomic_flag : public __atomic_flag_base
@@ -244,7 +249,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     atomic_flag& operator=(const atomic_flag&) volatile = delete;
 
     // Conversion to ATOMIC_FLAG_INIT.
-    atomic_flag(bool __i) noexcept : __atomic_flag_base({ __i }) { }
+    constexpr atomic_flag(bool __i) noexcept
+      : __atomic_flag_base({ __i ? __GCC_ATOMIC_TEST_AND_SET_TRUEVAL : 0 })
+    { }
 
     bool
     test_and_set(memory_order __m = memory_order_seq_cst) noexcept

[-- Attachment #5: z4 --]
[-- Type: text/plain, Size: 991 bytes --]

        * gcc.dg/atomic-flag.c: Adjust for __GCC_ATOMIC_TEST_AND_SET_TRUEVAL.


diff --git a/gcc/testsuite/gcc.dg/atomic-flag.c b/gcc/testsuite/gcc.dg/atomic-flag.c
index 771df2c..1b76832 100644
--- a/gcc/testsuite/gcc.dg/atomic-flag.c
+++ b/gcc/testsuite/gcc.dg/atomic-flag.c
@@ -1,27 +1,25 @@
 /* Test __atomic routines for existence and execution.  */
 /* { dg-do run } */
 
-#include <stdbool.h>
-
 /* Test that __atomic_test_and_set and __atomic_clear builtins execute.  */
 
 extern void abort(void);
-bool a;
+unsigned char a;
 
 main ()
 {
-  bool b;
+  int b;
 
   __atomic_clear (&a, __ATOMIC_RELAXED);
   if (a != 0)
     abort ();
 
   b = __atomic_test_and_set (&a, __ATOMIC_SEQ_CST);
-  if (a != 1 || b != 0)
+  if (a != __GCC_ATOMIC_TEST_AND_SET_TRUEVAL || b != 0)
     abort ();
 
   b = __atomic_test_and_set (&a, __ATOMIC_ACQ_REL);
-  if (b != 1 || a != 1)
+  if (a != __GCC_ATOMIC_TEST_AND_SET_TRUEVAL || b != 1)
     abort ();
 
   __atomic_clear (&a, __ATOMIC_SEQ_CST);

[-- Attachment #6: z5 --]
[-- Type: text/plain, Size: 2607 bytes --]

        * config/sparc/sparc.c (TARGET_ATOMIC_TEST_AND_SET_TRUEVAL): New.
        * config/sparc/sync.md (atomic_test_and_set): Only handle QImode.
        (ldstub): Rename from ldstubqi.
        (ldstub<I24MODE>): Remove.


diff --git a/gcc/config/sparc/sparc.c b/gcc/config/sparc/sparc.c
index 19ab54a..1b3b4c8 100644
--- a/gcc/config/sparc/sparc.c
+++ b/gcc/config/sparc/sparc.c
@@ -779,6 +779,10 @@ char sparc_hard_reg_printed[8];
 #undef TARGET_PRINT_OPERAND_ADDRESS
 #define TARGET_PRINT_OPERAND_ADDRESS sparc_print_operand_address
 
+/* The value stored by LDSTUB.  */
+#undef TARGET_ATOMIC_TEST_AND_SET_TRUEVAL
+#define TARGET_ATOMIC_TEST_AND_SET_TRUEVAL 0xff
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 
 static void
diff --git a/gcc/config/sparc/sync.md b/gcc/config/sparc/sync.md
index be8c4c4..d07d572 100644
--- a/gcc/config/sparc/sync.md
+++ b/gcc/config/sparc/sync.md
@@ -242,25 +242,30 @@
   "swap\t%1, %0"
   [(set_attr "type" "multi")])
 
-(define_expand "atomic_test_and_set<mode>"
-  [(match_operand:I124MODE 0 "register_operand" "")
-   (match_operand:I124MODE 1 "memory_operand" "")
+(define_expand "atomic_test_and_set"
+  [(match_operand:QI 0 "register_operand" "")
+   (match_operand:QI 1 "memory_operand" "")
    (match_operand:SI 2 "const_int_operand" "")]
   ""
 {
   enum memmodel model = (enum memmodel) INTVAL (operands[2]);
+  rtx ret;
 
   sparc_emit_membar_for_model (model, 3, 1);
+  emit_insn (gen_ldstub (operands[0], operands[1]));
+  sparc_emit_membar_for_model (model, 3, 2);
 
-  if (<MODE>mode != QImode)
-    operands[1] = adjust_address (operands[1], QImode, 0);
-  emit_insn (gen_ldstub<mode> (operands[0], operands[1]));
+  /* Convert the 0/0xff result we would otherwise have to a boolean.
+     I.e. ignore all but bit 0.  */
+  ret = expand_simple_binop (QImode, AND, operands[0], const1_rtx,
+			     operands[0], true, OPTAB_LIB_WIDEN);
+  if (ret != operands[0])
+    emit_move_insn (operands[0], ret);
 
-  sparc_emit_membar_for_model (model, 3, 2);
   DONE;
 })
 
-(define_insn "ldstubqi"
+(define_insn "ldstub"
   [(set (match_operand:QI 0 "register_operand" "=r")
 	(unspec_volatile:QI [(match_operand:QI 1 "memory_operand" "+m")]
 			    UNSPECV_LDSTUB))
@@ -268,13 +273,3 @@
   ""
   "ldstub\t%1, %0"
   [(set_attr "type" "multi")])
-
-(define_insn "ldstub<mode>"
-  [(set (match_operand:I24MODE 0 "register_operand" "=r")
-	(zero_extend:I24MODE
-	  (unspec_volatile:QI [(match_operand:QI 1 "memory_operand" "+m")]
-			      UNSPECV_LDSTUB)))
-   (set (match_dup 1) (const_int -1))]
-  ""
-  "ldstub\t%1, %0"
-  [(set_attr "type" "multi")])

[-- Attachment #7: z6 --]
[-- Type: text/plain, Size: 2180 bytes --]

        * config/m68k/m68k.c (TARGET_ATOMIC_TEST_AND_SET_TRUEVAL): New.
        * config/m68k/sync.md (atomic_test_and_set): Rename from
        sync_test_and_setqi and adjust the operands.
        (atomic_test_and_set_1): Rename from sync_test_and_setqi_1
        and unconditionally enable.


diff --git a/gcc/config/m68k/m68k.c b/gcc/config/m68k/m68k.c
index e0edd5b..d3ed82b 100644
--- a/gcc/config/m68k/m68k.c
+++ b/gcc/config/m68k/m68k.c
@@ -303,6 +303,10 @@ static void m68k_init_sync_libfuncs (void) ATTRIBUTE_UNUSED;
 #undef TARGET_ASM_OUTPUT_ADDR_CONST_EXTRA
 #define TARGET_ASM_OUTPUT_ADDR_CONST_EXTRA m68k_output_addr_const_extra
 
+/* The value stored by TAS.  */
+#undef TARGET_ATOMIC_TEST_AND_SET_TRUEVAL
+#define TARGET_ATOMIC_TEST_AND_SET_TRUEVAL 128
+
 static const struct attribute_spec m68k_attribute_table[] =
 {
   /* { name, min_len, max_len, decl_req, type_req, fn_type_req, handler,
diff --git a/gcc/config/m68k/sync.md b/gcc/config/m68k/sync.md
index a40a5bf..5d5002a 100644
--- a/gcc/config/m68k/sync.md
+++ b/gcc/config/m68k/sync.md
@@ -56,25 +56,23 @@
   ;; Elide the seq if operands[0] is dead.
   "cas<sz> %1,%4,%2\;seq %0")
 
-(define_expand "sync_test_and_setqi"
-  [(match_operand:QI 0 "register_operand" "")
-   (match_operand:QI 1 "memory_operand" "")
-   (match_operand:QI 2 "general_operand" "")]
-  "!TARGET_CAS"
+(define_expand "atomic_test_and_set"
+  [(match_operand:QI 0 "register_operand" "")		;; bool success output
+   (match_operand:QI 1 "memory_operand" "")		;; memory
+   (match_operand:SI 2 "const_int_operand" "")]		;; model
+  ""
 {
-  if (operands[2] != const1_rtx)
-    FAIL;
-  emit_insn (gen_sync_test_and_setqi_1 (operands[0], operands[1]));
+  emit_insn (gen_atomic_test_and_set_1 (operands[0], operands[1]));
   emit_insn (gen_negqi2 (operands[0], operands[0]));
   DONE;
 })
 
-(define_insn "sync_test_and_setqi_1"
+(define_insn "atomic_test_and_set_1"
   [(set (match_operand:QI 0 "register_operand" "=d")
 	(unspec_volatile:QI
 	  [(match_operand:QI 1 "memory_operand" "+m")]
 	  UNSPECV_TAS_1))
    (set (match_dup 1)
 	(unspec_volatile:QI [(match_dup 1)] UNSPECV_TAS_2))]
-  "!TARGET_CAS"
+  ""
   "tas %1\;sne %0")

Re: [m68k, sparc] Fix atomic_test_and_set

[David Miller]

From: Richard Henderson <rth@redhat.com>
Date: Fri, 27 Jan 2012 09:29:00 +1100

> Two of the patches have been posted here before; the libstdc++
> patch was approved by Benjamin.
> 
> All of the patches tested on sparc64-linux, and sanity checked
> on x86_64-linux.  I've cross-compiled for m68k-linux, but I've
> only been able to visually sanity check the code in libstdc++.
> 
> Committed.  Hopefully that wraps up the atomic patches...

Why do you need to define TARGET_ATOMIC_TEST_AND_SET_TRUEVAL to 0xff
on sparc if you're going to convert the result into a bool in the
expander?

Re: [m68k, sparc] Fix atomic_test_and_set

[Richard Henderson]

On 01/27/2012 09:34 AM, David Miller wrote:
> From: Richard Henderson <rth@redhat.com>
> Date: Fri, 27 Jan 2012 09:29:00 +1100
> 
>> Two of the patches have been posted here before; the libstdc++
>> patch was approved by Benjamin.
>>
>> All of the patches tested on sparc64-linux, and sanity checked
>> on x86_64-linux.  I've cross-compiled for m68k-linux, but I've
>> only been able to visually sanity check the code in libstdc++.
>>
>> Committed.  Hopefully that wraps up the atomic patches...
> 
> Why do you need to define TARGET_ATOMIC_TEST_AND_SET_TRUEVAL to 0xff
> on sparc if you're going to convert the result into a bool in the
> expander?

Because the value stored in memory is 0xff, and not a boolean.
The boolean is only the result type of __atomic_test_and_set.


r~

Re: [m68k, sparc] Fix atomic_test_and_set

[David Miller]

From: Richard Henderson <rth@redhat.com>
Date: Fri, 27 Jan 2012 09:41:10 +1100

> On 01/27/2012 09:34 AM, David Miller wrote:
>> From: Richard Henderson <rth@redhat.com>
>> Date: Fri, 27 Jan 2012 09:29:00 +1100
>> 
>>> Two of the patches have been posted here before; the libstdc++
>>> patch was approved by Benjamin.
>>>
>>> All of the patches tested on sparc64-linux, and sanity checked
>>> on x86_64-linux.  I've cross-compiled for m68k-linux, but I've
>>> only been able to visually sanity check the code in libstdc++.
>>>
>>> Committed.  Hopefully that wraps up the atomic patches...
>> 
>> Why do you need to define TARGET_ATOMIC_TEST_AND_SET_TRUEVAL to 0xff
>> on sparc if you're going to convert the result into a bool in the
>> expander?
> 
> Because the value stored in memory is 0xff, and not a boolean.
> The boolean is only the result type of __atomic_test_and_set.

Gotcha, the changes thus look fine to me.

Re: [m68k, sparc] Fix atomic_test_and_set

[Oleg Endo]

[-- Attachment #1: Type: text/plain, Size: 1470 bytes --]

On Fri, 2012-01-27 at 09:29 +1100, Richard Henderson wrote:
> Two of the patches have been posted here before; the libstdc++
> patch was approved by Benjamin.
> 
> All of the patches tested on sparc64-linux, and sanity checked
> on x86_64-linux.  I've cross-compiled for m68k-linux, but I've
> only been able to visually sanity check the code in libstdc++.
> 
> Committed.  Hopefully that wraps up the atomic patches...
> 

Wouldn't it make sense to use the value behind
TARGET_ATOMIC_TEST_AND_SET_TRUEVAL in optabs.c
(expand_atomic_test_and_set) instead of const1_rtx when emitting
generated atomic_exchange / atomic_compare_and_swap_exchange_loop?
Maybe something like the attached patch?

Background:
I'm working on a patch to add a new option -menable-tas (independent of
the existing -msoft-atomic option) which would allow the compiler to
generate SH's tas.b insn.  This would allow using the tas.b insn without
the other atomic sequences, or in combination with them.  The reason
behind this is that the tas.b insn might not always be appropriate to
use, depending on the particular system hardware/software setup (e.g.
dual-core SH4A).
On SH the TARGET_ATOMIC_TEST_AND_SET_TRUEVAL has to be defined as 0x80.
Having the generated atomic_compare_and_swap / atomic_exchange sequences
using 0x01 as the 'set' value might lead to inconsistencies when mixing
code that uses the tas.b insn and code that doesn't use it, which should
actually be OK to do.

Cheers,
Oleg

[-- Attachment #2: tas_trueval.patch --]
[-- Type: text/x-patch, Size: 1362 bytes --]

Index: gcc/optabs.c
===================================================================
--- gcc/optabs.c	(revision 184589)
+++ gcc/optabs.c	(working copy)
@@ -7384,6 +7384,7 @@
 expand_atomic_test_and_set (rtx target, rtx mem, enum memmodel model)
 {
   enum machine_mode mode = GET_MODE (mem);
+  rtx tas_trueval = GEN_INT (targetm.atomic_test_and_set_trueval);
   rtx ret;
 
   ret = maybe_emit_atomic_test_and_set (target, mem, model);
@@ -7395,22 +7396,22 @@
 
   /* 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);
+  ret = maybe_emit_atomic_exchange (target, mem, tas_trueval, model);
   if (ret)
     return ret;
 
-  ret = maybe_emit_compare_and_swap_exchange_loop (target, mem, const1_rtx);
+  ret = maybe_emit_compare_and_swap_exchange_loop (target, mem, tas_trueval);
   if (ret)
     return ret;
 
-  ret = maybe_emit_sync_lock_test_and_set (target, mem, const1_rtx, model);
+  ret = maybe_emit_sync_lock_test_and_set (target, mem, tas_trueval, model);
   if (ret)
     return ret;
 
   /* Failing all else, assume a single threaded environment and simply perform
      the operation.  */
   emit_move_insn (target, mem);
-  emit_move_insn (mem, const1_rtx);
+  emit_move_insn (mem, tas_trueval);
   return target;
 }
 

[4.7][SH] Binary compatibility with atomic_test_and_test_trueval != 1

[Richard Henderson]

[-- Attachment #1: Type: text/plain, Size: 1750 bytes --]

On 02/28/2012 07:16 AM, Oleg Endo wrote:
> Wouldn't it make sense to use the value behind
> TARGET_ATOMIC_TEST_AND_SET_TRUEVAL in optabs.c
> (expand_atomic_test_and_set) instead of const1_rtx when emitting
> generated atomic_exchange / atomic_compare_and_swap_exchange_loop?
> Maybe something like the attached patch?
> 
> Background:
> I'm working on a patch to add a new option -menable-tas (independent of
> the existing -msoft-atomic option) which would allow the compiler to
> generate SH's tas.b insn.  This would allow using the tas.b insn without
> the other atomic sequences, or in combination with them.  The reason
> behind this is that the tas.b insn might not always be appropriate to
> use, depending on the particular system hardware/software setup (e.g.
> dual-core SH4A).
> On SH the TARGET_ATOMIC_TEST_AND_SET_TRUEVAL has to be defined as 0x80.
> Having the generated atomic_compare_and_swap / atomic_exchange sequences
> using 0x01 as the 'set' value might lead to inconsistencies when mixing
> code that uses the tas.b insn and code that doesn't use it, which should
> actually be OK to do.

That's a good idea.

For proper binary compatibility, we should probably introduce that asap.
You forgot two things in your patch, gen_int_mode and the fact that we
still have to return a boolean (0/1) value.

Also in order for the binary compatibility to work right, you'd want to
have the SH test-and-set-trueval set appropriately asap.  Kaz, I assume
you'd agree that 0x80 is a good value for the "tas.b" insn?  We don't
necessarily need to support tas.b right away, but getting trueval set
right is imperative.

I'm in the process of sanity testing this on x86_64 with trueval set to 0x80.
Jakub, ok for 4.7 branch if it passes?


r~

[-- Attachment #2: z --]
[-- Type: text/plain, Size: 2952 bytes --]

	* optabs.c (expand_atomic_test_and_set): Honor
	atomic_test_and_set_trueval even when atomic_test_and_set
	optab is not in use.



diff --git a/gcc/optabs.c b/gcc/optabs.c
index b0ecdf0..fd353d7 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -7384,34 +7384,57 @@ rtx
 expand_atomic_test_and_set (rtx target, rtx mem, enum memmodel model)
 {
   enum machine_mode mode = GET_MODE (mem);
-  rtx ret;
+  rtx ret, trueval, subtarget;
 
   ret = maybe_emit_atomic_test_and_set (target, mem, model);
   if (ret)
     return ret;
 
-  if (target == NULL_RTX)
-    target = gen_reg_rtx (mode);
+  /* Be binary compatible with non-default settings of trueval, and different
+     cpu revisions.  E.g. one revision may have atomic-test-and-set, but
+     another only has atomic-exchange.  */
+  if (targetm.atomic_test_and_set_trueval == 1)
+    {
+      trueval = const1_rtx;
+      subtarget = target ? target : gen_reg_rtx (mode);
+    }
+  else
+    {
+      trueval = gen_int_mode (targetm.atomic_test_and_set_trueval, mode);
+      subtarget = gen_reg_rtx (mode);
+    }
 
-  /* 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;
+  /* Try the atomic-exchange optab...  */
+  ret = maybe_emit_atomic_exchange (subtarget, mem, trueval, model);
 
-  ret = maybe_emit_compare_and_swap_exchange_loop (target, mem, const1_rtx);
-  if (ret)
-    return ret;
+  /* ... then an atomic-compare-and-swap loop ... */
+  if (!ret)
+    ret = maybe_emit_compare_and_swap_exchange_loop (subtarget, mem, trueval);
 
-  ret = maybe_emit_sync_lock_test_and_set (target, mem, const1_rtx, model);
-  if (ret)
-    return ret;
+  /* ... before trying the vaguely defined legacy lock_test_and_set. */
+  if (!ret)
+    ret = maybe_emit_sync_lock_test_and_set (subtarget, mem, trueval, model);
 
-  /* Failing all else, assume a single threaded environment and simply perform
-     the operation.  */
-  emit_move_insn (target, mem);
-  emit_move_insn (mem, const1_rtx);
-  return target;
+  /* Recall that the legacy lock_test_and_set optab was allowed to do magic
+     things with the value 1.  Thus we try again without trueval.  */
+  if (!ret && targetm.atomic_test_and_set_trueval != 1)
+    ret = maybe_emit_sync_lock_test_and_set (subtarget, mem, const1_rtx, model);
+
+  /* Failing all else, assume a single threaded environment and simply
+     perform the operation.  */
+  if (!ret)
+    {
+      emit_move_insn (subtarget, mem);
+      emit_move_insn (mem, trueval);
+      ret = subtarget;
+    }
+
+  /* Recall that have to return a boolean value; rectify if trueval
+     is not exactly one.  */
+  if (targetm.atomic_test_and_set_trueval != 1)
+    ret = emit_store_flag_force (target, NE, ret, const0_rtx, mode, 0, 1);
+  
+  return ret;
 }
 
 /* This function expands the atomic exchange operation:

Re: [4.7][SH] Binary compatibility with atomic_test_and_test_trueval != 1

[Oleg Endo]

[-- Attachment #1: Type: text/plain, Size: 1024 bytes --]

On Fri, 2012-03-02 at 10:11 -0800, Richard Henderson wrote:

> For proper binary compatibility, we should probably introduce that asap.
> You forgot two things in your patch, gen_int_mode and the fact that we
> still have to return a boolean (0/1) value.

Ah, yes, of course!

> 
> Also in order for the binary compatibility to work right, you'd want to
> have the SH test-and-set-trueval set appropriately asap.  Kaz, I assume
> you'd agree that 0x80 is a good value for the "tas.b" insn?  We don't
> necessarily need to support tas.b right away, but getting trueval set
> right is imperative.
> 
> I'm in the process of sanity testing this on x86_64 with trueval set to 0x80.
> Jakub, ok for 4.7 branch if it passes?
> 

Since we're now back to stage 1 again, I'm about to commit this one ...
http://gcc.gnu.org/ml/gcc-patches/2012-03/msg00085.html

So the SH tas.b instruction support should be there from 4.8.
Attached is the hunk from the tas.b patch for 4.8 to define
TARGET_ATOMIC_TEST_AND_SET_TRUEVAL.

Cheers,
Oleg

[-- Attachment #2: sh_tasb_trueval.patch --]
[-- Type: text/x-patch, Size: 726 bytes --]

Index: gcc/config/sh/sh.h
===================================================================
--- gcc/config/sh/sh.h	(revision 184669)
+++ gcc/config/sh/sh.h	(working copy)
@@ -2475,4 +2475,11 @@
 /* FIXME: middle-end support for highpart optimizations is missing.  */
 #define high_life_started reload_in_progress
 
+/* The tas.b instruction sets the 7th bit in the byte, i.e. 0x80.
+   This value is used by optabs.c atomic op expansion code as well as in 
+   sync.md.  It must be defined as signed char here or else the movqi
+   pattern will refuse to load it as a QImode constant.  */
+#undef TARGET_ATOMIC_TEST_AND_SET_TRUEVAL
+#define TARGET_ATOMIC_TEST_AND_SET_TRUEVAL ((signed char)0x80)
+
 #endif /* ! GCC_SH_H */

Re: [4.7][SH] Binary compatibility with atomic_test_and_test_trueval != 1

[Richard Henderson]

On 03/02/2012 12:27 PM, Oleg Endo wrote:
> +   This value is used by optabs.c atomic op expansion code as well as in 
> +   sync.md.  It must be defined as signed char here or else the movqi
> +   pattern will refuse to load it as a QImode constant.  */
> +#undef TARGET_ATOMIC_TEST_AND_SET_TRUEVAL
> +#define TARGET_ATOMIC_TEST_AND_SET_TRUEVAL ((signed char)0x80)

The fix to use gen_mode_int obviates the need for the (signed char) hack.


r~

Re: [4.7][SH] Binary compatibility with atomic_test_and_test_trueval != 1

[Oleg Endo]

On Fri, 2012-03-02 at 12:34 -0800, Richard Henderson wrote:
> On 03/02/2012 12:27 PM, Oleg Endo wrote:
> > +   This value is used by optabs.c atomic op expansion code as well as in 
> > +   sync.md.  It must be defined as signed char here or else the movqi
> > +   pattern will refuse to load it as a QImode constant.  */
> > +#undef TARGET_ATOMIC_TEST_AND_SET_TRUEVAL
> > +#define TARGET_ATOMIC_TEST_AND_SET_TRUEVAL ((signed char)0x80)
> 
> The fix to use gen_mode_int obviates the need for the (signed char) hack.
> 

Ah right, sorry again.
Then I'll rather wait with the whole tas.b patch until the optabs
changes are in.

Cheers,
Oleg

Re: [4.7][SH] Binary compatibility with atomic_test_and_test_trueval != 1

[Kaz Kojima]

Richard Henderson <rth@redhat.com> wrote:
> For proper binary compatibility, we should probably introduce that asap.
> You forgot two things in your patch, gen_int_mode and the fact that we
> still have to return a boolean (0/1) value.
> 
> Also in order for the binary compatibility to work right, you'd want to
> have the SH test-and-set-trueval set appropriately asap.  Kaz, I assume
> you'd agree that 0x80 is a good value for the "tas.b" insn?  We don't
> necessarily need to support tas.b right away, but getting trueval set
> right is imperative.

Yes, 0x80 is an appropriate value as you and oleg have suggested.

Regards,
	kaz

Re: [4.7][SH] Binary compatibility with atomic_test_and_test_trueval != 1

[Richard Henderson]

On 03/02/2012 10:11 AM, Richard Henderson wrote:
> I'm in the process of sanity testing this on x86_64 with trueval set to 0x80.
> Jakub, ok for 4.7 branch if it passes?
> 
> 	* optabs.c (expand_atomic_test_and_set): Honor
> 	atomic_test_and_set_trueval even when atomic_test_and_set
> 	optab is not in use.

I've committed this patch to mainline.  I still think it ought to 
go onto the 4.7 branch...


r~

Re: [4.7][SH] Binary compatibility with atomic_test_and_test_trueval != 1

[Jakub Jelinek]

On Sat, Mar 03, 2012 at 10:31:17AM -0800, Richard Henderson wrote:
> On 03/02/2012 10:11 AM, Richard Henderson wrote:
> > I'm in the process of sanity testing this on x86_64 with trueval set to 0x80.
> > Jakub, ok for 4.7 branch if it passes?
> > 
> > 	* optabs.c (expand_atomic_test_and_set): Honor
> > 	atomic_test_and_set_trueval even when atomic_test_and_set
> > 	optab is not in use.
> 
> I've committed this patch to mainline.  I still think it ought to 
> go onto the 4.7 branch...

Ok.

	Jakub

Re: [4.7][SH] Binary compatibility with atomic_test_and_test_trueval != 1

[Oleg Endo]

[-- Attachment #1: Type: text/plain, Size: 1366 bytes --]

On Sat, 2012-03-03 at 10:31 -0800, Richard Henderson wrote:
> On 03/02/2012 10:11 AM, Richard Henderson wrote:
> > I'm in the process of sanity testing this on x86_64 with trueval set to 0x80.
> > Jakub, ok for 4.7 branch if it passes?
> > 
> > 	* optabs.c (expand_atomic_test_and_set): Honor
> > 	atomic_test_and_set_trueval even when atomic_test_and_set
> > 	optab is not in use.
> 
> I've committed this patch to mainline.  I still think it ought to 
> go onto the 4.7 branch...
> 

Attached is a slightly modified version of the patch from
http://gcc.gnu.org/ml/gcc-patches/2012-03/msg00085.html

I have removed the signed char weirdo and adjusted the comment above
TARGET_ATOMIC_TEST_AND_SET_TRUEVAL accordingly.

Tested by compiling some test functions that use __atomic_test_and_set /
__GCC_ATOMIC_TEST_AND_SET_TRUEVAL with various SH atomic option
combinations and looking at the output asm.

OK to apply to trunk?

Richard, could you also please take the
TARGET_ATOMIC_TEST_AND_SET_TRUEVAL hunk from this patch for the 4.7
branch?

Cheers,
Oleg


2012-03-04  Oleg Endo  <olegendo@gcc.gnu.org>

	* config/sh/sh.h (TARGET_ATOMIC_TEST_AND_SET_TRUEVAL): New hook.
	* config/sh/sync.md (atomic_test_and_set): New expander.
	(tasb, atomic_test_and_set_soft): New insns.
	* config/sh/sh.opt (menable-tas): New option.
	* doc/invoke.texi (SH Options): Document it.

[-- Attachment #2: 05_sh_atomic_tas.patch --]
[-- Type: text/x-patch, Size: 4583 bytes --]

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 184877)
+++ gcc/doc/invoke.texi	(working copy)
@@ -887,7 +887,8 @@
 -mdivsi3_libfunc=@var{name} -mfixed-range=@var{register-range} @gol
 -madjust-unroll -mindexed-addressing -mgettrcost=@var{number} -mpt-fixed @gol
 -maccumulate-outgoing-args -minvalid-symbols -msoft-atomic @gol
--mbranch-cost=@var{num} -mcbranchdi -mcmpeqdi -mfused-madd -mpretend-cmove}
+-mbranch-cost=@var{num} -mcbranchdi -mcmpeqdi -mfused-madd -mpretend-cmove @gol
+-menable-tas}
 
 @emph{Solaris 2 Options}
 @gccoptlist{-mimpure-text  -mno-impure-text @gol
@@ -17823,6 +17824,15 @@
 This option is enabled by default when the target is @code{sh-*-linux*}.
 For details on the atomic built-in functions see @ref{__atomic Builtins}.
 
+@item -menable-tas
+@opindex menable-tas
+Generate the @code{tas.b} opcode for @code{__atomic_test_and_set}.
+Notice that depending on the particular hardware and software configuration
+this can degrade overall performance due to the operand cache line flushes
+that are implied by the @code{tas.b} instruction.  On multi-core SH4A
+processors the @code{tas.b} instruction must be used with caution since it
+can result in data corruption for certain cache configurations.
+
 @item -mspace
 @opindex mspace
 Optimize for space instead of speed.  Implied by @option{-Os}.
Index: gcc/config/sh/sh.h
===================================================================
--- gcc/config/sh/sh.h	(revision 184877)
+++ gcc/config/sh/sh.h	(working copy)
@@ -2473,4 +2473,10 @@
 /* FIXME: middle-end support for highpart optimizations is missing.  */
 #define high_life_started reload_in_progress
 
+/* The tas.b instruction sets the 7th bit in the byte, i.e. 0x80.
+   This value is used by optabs.c atomic op expansion code as well as in 
+   sync.md.  */
+#undef TARGET_ATOMIC_TEST_AND_SET_TRUEVAL
+#define TARGET_ATOMIC_TEST_AND_SET_TRUEVAL 0x80
+
 #endif /* ! GCC_SH_H */
Index: gcc/config/sh/sync.md
===================================================================
--- gcc/config/sh/sync.md	(revision 184877)
+++ gcc/config/sh/sync.md	(working copy)
@@ -404,3 +404,61 @@
 	 "1:	mov	r1,r15";
 }
   [(set_attr "length" "18")])
+
+(define_expand "atomic_test_and_set"
+  [(match_operand:SI 0 "register_operand" "")		;; bool result output
+   (match_operand:QI 1 "memory_operand" "")		;; memory
+   (match_operand:SI 2 "const_int_operand" "")]		;; model
+  "(TARGET_SOFT_ATOMIC || TARGET_ENABLE_TAS) && !TARGET_SHMEDIA"
+{
+  rtx addr = force_reg (Pmode, XEXP (operands[1], 0));
+
+  if (TARGET_ENABLE_TAS)
+    emit_insn (gen_tasb (addr));
+  else
+    {
+      rtx val = force_reg (QImode, 
+			   gen_int_mode (TARGET_ATOMIC_TEST_AND_SET_TRUEVAL,
+					 QImode));
+      emit_insn (gen_atomic_test_and_set_soft (addr, val));
+    }
+
+  /* The result of the test op is the inverse of what we are
+     supposed to return.  Thus invert the T bit.  The inversion will be
+     potentially optimized away and integrated into surrounding code.  */
+  emit_insn (gen_movnegt (operands[0]));
+  DONE;
+})
+
+(define_insn "tasb"
+  [(set (reg:SI T_REG)
+	(eq:SI (mem:QI (match_operand:SI 0 "register_operand" "r"))
+	       (const_int 0)))
+   (set (mem:QI (match_dup 0))
+	(unspec:QI [(const_int 128)] UNSPEC_ATOMIC))]
+  "TARGET_ENABLE_TAS && !TARGET_SHMEDIA"
+  "tas.b	@%0"
+  [(set_attr "insn_class" "co_group")])
+
+(define_insn "atomic_test_and_set_soft"
+  [(set (reg:SI T_REG)
+	(eq:SI (mem:QI (match_operand:SI 0 "register_operand" "u"))
+	       (const_int 0)))
+   (set (mem:QI (match_dup 0))
+	(unspec:QI [(match_operand:QI 1 "register_operand" "u")] UNSPEC_ATOMIC))
+   (clobber (match_scratch:QI 2 "=&u"))
+   (clobber (reg:SI R0_REG))
+   (clobber (reg:SI R1_REG))]
+  "TARGET_SOFT_ATOMIC && !TARGET_ENABLE_TAS && !TARGET_SHMEDIA"
+{
+  return "mova	1f,r0"			"\n"
+	 "	.align 2"		"\n"
+	 "	mov	r15,r1"		"\n"
+	 "	mov	#(0f-1f),r15"	"\n"
+	 "0:	mov.b	@%0,%2"		"\n"
+	 "	mov.b	%1,@%0"		"\n"
+	 "1:	mov	r1,r15"		"\n"
+	 "	tst	%2,%2";
+}
+  [(set_attr "length" "16")])
+
Index: gcc/config/sh/sh.opt
===================================================================
--- gcc/config/sh/sh.opt	(revision 184877)
+++ gcc/config/sh/sh.opt	(working copy)
@@ -323,6 +323,10 @@
 Target Report Mask(SOFT_ATOMIC)
 Use software atomic sequences supported by kernel
 
+menable-tas
+Target Report RejectNegative Var(TARGET_ENABLE_TAS)
+Use tas.b instruction for __atomic_test_and_set
+
 mspace
 Target RejectNegative Alias(Os)
 Deprecated.  Use -Os instead

Re: [4.7][SH] Binary compatibility with atomic_test_and_test_trueval != 1

[Kaz Kojima]

Oleg Endo <oleg.endo@t-online.de> wrote:
> Attached is a slightly modified version of the patch from
> http://gcc.gnu.org/ml/gcc-patches/2012-03/msg00085.html
> 
> I have removed the signed char weirdo and adjusted the comment above
> TARGET_ATOMIC_TEST_AND_SET_TRUEVAL accordingly.
> 
> Tested by compiling some test functions that use __atomic_test_and_set /
> __GCC_ATOMIC_TEST_AND_SET_TRUEVAL with various SH atomic option
> combinations and looking at the output asm.
> 
> OK to apply to trunk?

OK.

Regards,
	kaz

Re: [4.7][SH] Binary compatibility with atomic_test_and_test_trueval != 1

[Richard Henderson]

On 03/04/2012 11:09 AM, Oleg Endo wrote:
> Richard, could you also please take the
> TARGET_ATOMIC_TEST_AND_SET_TRUEVAL hunk from this patch for the 4.7
> branch?

Done.  I've also moved the TARGET_ATOMIC_TEST_AND_SET_TRUEVAL 
definition from sh.h to sh.c where it belongs.


r~

Re: [4.7][SH] Binary compatibility with atomic_test_and_test_trueval != 1

[Oleg Endo]

On Mon, 2012-03-05 at 11:00 -0800, Richard Henderson wrote:
> On 03/04/2012 11:09 AM, Oleg Endo wrote:
> > Richard, could you also please take the
> > TARGET_ATOMIC_TEST_AND_SET_TRUEVAL hunk from this patch for the 4.7
> > branch?
> 
> Done.  

Thanks!

> I've also moved the TARGET_ATOMIC_TEST_AND_SET_TRUEVAL 
> definition from sh.h to sh.c where it belongs.

Yeah, however, I'm also using the value behind
TARGET_ATOMIC_TEST_AND_SET_TRUEVAL in sync.md.  If it's in sh.c it
doesn't work.  That's why I left it in sh.h.

Cheers,
Oleg

Re: [4.7][SH] Binary compatibility with atomic_test_and_test_trueval != 1

[Richard Henderson]

On 03/05/2012 01:44 PM, Oleg Endo wrote:
> Yeah, however, I'm also using the value behind
> TARGET_ATOMIC_TEST_AND_SET_TRUEVAL in sync.md.  If it's in sh.c it
> doesn't work.  That's why I left it in sh.h.

That value should be available via targetm.atomic_test_and_set_trueval.


r~

Re: [4.7][SH] Binary compatibility with atomic_test_and_test_trueval != 1

[Oleg Endo]

On Mon, 2012-03-05 at 13:49 -0800, Richard Henderson wrote:
> On 03/05/2012 01:44 PM, Oleg Endo wrote:
> > Yeah, however, I'm also using the value behind
> > TARGET_ATOMIC_TEST_AND_SET_TRUEVAL in sync.md.  If it's in sh.c it
> > doesn't work.  That's why I left it in sh.h.
> 
> That value should be available via targetm.atomic_test_and_set_trueval.
> 

Just tried out in sync.md ...

-   gen_int_mode (TARGET_ATOMIC_TEST_AND_SET_TRUEVAL,
+   gen_int_mode (targetm.atomic_test_and_set_trueval,

gives .. 
/gcc/config/sh/sync.md:421:21: error: ‘targetm’ undeclared

Is there something missing?

Re: [4.7][SH] Binary compatibility with atomic_test_and_test_trueval != 1

[Richard Henderson]

[-- Attachment #1: Type: text/plain, Size: 355 bytes --]

On 03/05/2012 01:49 PM, Richard Henderson wrote:
> On 03/05/2012 01:44 PM, Oleg Endo wrote:
>> Yeah, however, I'm also using the value behind
>> TARGET_ATOMIC_TEST_AND_SET_TRUEVAL in sync.md.  If it's in sh.c it
>> doesn't work.  That's why I left it in sh.h.
> 
> That value should be available via targetm.atomic_test_and_set_trueval.

Fixed thus.


r~

[-- Attachment #2: z --]
[-- Type: text/plain, Size: 1978 bytes --]

diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 9a35295..0b7e635 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -3495,8 +3495,8 @@ insn-automata.o : insn-automata.c $(CONFIG_H) $(SYSTEM_H) coretypes.h	\
 insn-emit.o : insn-emit.c $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H)	\
   $(RTL_H) $(TM_P_H) $(FUNCTION_H) $(EXPR_H) $(OPTABS_H) 		\
   dfp.h $(FLAGS_H) output.h insn-config.h hard-reg-set.h $(RECOG_H)	\
-  $(RESOURCE_H) reload.h $(DIAGNOSTIC_CORE_H) $(REGS_H) tm-constrs.h $(GGC_H)	\
-  $(BASIC_BLOCK_H) $(INTEGRATE_H)
+  $(RESOURCE_H) reload.h $(DIAGNOSTIC_CORE_H) $(REGS_H) tm-constrs.h	\
+  $(GGC_H) $(BASIC_BLOCK_H) $(INTEGRATE_H) $(TARGET_H)
 insn-enums.o : insn-enums.c $(CONFIG_H) $(SYSTEM_H) insn-constants.h
 insn-extract.o : insn-extract.c $(CONFIG_H) $(SYSTEM_H) coretypes.h	\
   $(TM_H) $(RTL_H) $(DIAGNOSTIC_CORE_H) insn-config.h $(RECOG_H)
diff --git a/gcc/config/sh/sync.md b/gcc/config/sh/sync.md
index 113288c..258e048 100644
--- a/gcc/config/sh/sync.md
+++ b/gcc/config/sh/sync.md
@@ -417,9 +417,10 @@
     emit_insn (gen_tasb (addr));
   else
     {
-      rtx val = force_reg (QImode, 
-			   gen_int_mode (TARGET_ATOMIC_TEST_AND_SET_TRUEVAL,
-					 QImode));
+      rtx val;
+
+      val = gen_int_mode (targetm.atomic_test_and_set_trueval, QImode);
+      val = force_reg (QImode, val);
       emit_insn (gen_atomic_test_and_set_soft (addr, val));
     }
 
diff --git a/gcc/genemit.c b/gcc/genemit.c
index 662d8ca..173e4d3 100644
--- a/gcc/genemit.c
+++ b/gcc/genemit.c
@@ -812,7 +812,8 @@ from the machine description file `md'.  */\n\n");
   printf ("#include \"tm-constrs.h\"\n");
   printf ("#include \"ggc.h\"\n");
   printf ("#include \"basic-block.h\"\n");
-  printf ("#include \"integrate.h\"\n\n");
+  printf ("#include \"integrate.h\"\n");
+  printf ("#include \"target.h\"\n\n");
   printf ("#define FAIL return (end_sequence (), _val)\n");
   printf ("#define DONE return (_val = get_insns (), end_sequence (), _val)\n\n");