diff mbox

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

Message ID 4F510D55.4090803@redhat.com
State New
Headers show

Commit Message

Richard Henderson March 2, 2012, 6:11 p.m. UTC 
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~
* optabs.c (expand_atomic_test_and_set): Honor
	atomic_test_and_set_trueval even when atomic_test_and_set
	optab is not in use.

Comments

Kaz Kojima March 2, 2012, 11:39 p.m. UTC | #1 
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
Richard Henderson March 3, 2012, 6:31 p.m. UTC | #2 
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~
Jakub Jelinek March 3, 2012, 7:23 p.m. UTC | #3 
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
diff mbox

Patch

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: