Patchwork [rs6000] Streamline compare-and-swap success return value computation

login
register
mail settings
Submitter Richard Henderson
Date Nov. 29, 2011, 12:33 a.m.
Message ID <4ED42876.8030704@redhat.com>
Download mbox | patch
Permalink /patch/128174/
State New
Headers show

Comments

Richard Henderson - Nov. 29, 2011, 12:33 a.m.
On 11/28/2011 04:26 PM, Richard Henderson wrote:
> On 11/28/2011 03:05 PM, Richard Henderson wrote:
>> On 11/28/2011 02:16 PM, Alan Modra wrote:
>>> Hmm, I suppose you could argue that powerpc and others ought to not
>>> generate those three extra instructions when using the return value.
>>> I'll see about fixing powerpc.
>>
>> However, we can do better by considering the value to be stored in CR0...
> 
> Try this and see if it generates the sort of code you want.  Untested.

... actually, this one.  There's no reason to differentiate between strong
and weak compare-and-swap when computing boolval.


r~
Alan Modra - Nov. 29, 2011, 1:07 a.m.
On Mon, Nov 28, 2011 at 04:33:58PM -0800, Richard Henderson wrote:
> On 11/28/2011 04:26 PM, Richard Henderson wrote:
> > On 11/28/2011 03:05 PM, Richard Henderson wrote:
> >> On 11/28/2011 02:16 PM, Alan Modra wrote:
> >>> Hmm, I suppose you could argue that powerpc and others ought to not
> >>> generate those three extra instructions when using the return value.
> >>> I'll see about fixing powerpc.
> >>
> >> However, we can do better by considering the value to be stored in CR0...
> > 
> > Try this and see if it generates the sort of code you want.  Untested.
> 
> ... actually, this one.  There's no reason to differentiate between strong
> and weak compare-and-swap when computing boolval.

Looks very similar to what I wrote, except

> +  else if (reg_overlap_mentioned_p (retval, oldval))
> +    oldval = copy_to_reg (oldval);

I missed this bit

> -  x = gen_rtx_NE (VOIDmode, x, oldval);
> -  x = rs6000_generate_compare (x, mode);
> +  cond = gen_reg_rtx (CCmode);
> +  x = gen_rtx_COMPARE (CCmode, x, oldval);
> +  emit_insn (gen_rtx_SET (VOIDmode, cond, x));
> +
> +  x = gen_rtx_NE (VOIDmode, cond, const0_rtx);

and instead pulled cond out of the return from rs6000_generate_compare.

Results look good.
David Edelsohn - Nov. 29, 2011, 3:13 p.m.
On Mon, Nov 28, 2011 at 7:33 PM, Richard Henderson <rth@redhat.com> wrote:
> On 11/28/2011 04:26 PM, Richard Henderson wrote:
>> On 11/28/2011 03:05 PM, Richard Henderson wrote:
>>> On 11/28/2011 02:16 PM, Alan Modra wrote:
>>>> Hmm, I suppose you could argue that powerpc and others ought to not
>>>> generate those three extra instructions when using the return value.
>>>> I'll see about fixing powerpc.
>>>
>>> However, we can do better by considering the value to be stored in CR0...
>>
>> Try this and see if it generates the sort of code you want.  Untested.
>
> ... actually, this one.  There's no reason to differentiate between strong
> and weak compare-and-swap when computing boolval.

Has anyone bootstrapped and regression-tested the patch?

Thanks, David
Richard Henderson - Nov. 29, 2011, 3:50 p.m.
On 11/29/2011 07:13 AM, David Edelsohn wrote:
>> ... actually, this one.  There's no reason to differentiate between strong
>> and weak compare-and-swap when computing boolval.
> 
> Has anyone bootstrapped and regression-tested the patch?

Yes, I did so last night on gcc110.


r~
David Edelsohn - Nov. 29, 2011, 4:17 p.m.
On Tue, Nov 29, 2011 at 10:50 AM, Richard Henderson <rth@redhat.com> wrote:
> On 11/29/2011 07:13 AM, David Edelsohn wrote:
>>> ... actually, this one.  There's no reason to differentiate between strong
>>> and weak compare-and-swap when computing boolval.
>>
>> Has anyone bootstrapped and regression-tested the patch?
>
> Yes, I did so last night on gcc110.

Great.  Then the patch is okay with me, if you have not committed it already.

Thanks!
David

Patch

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index f01353b..5a33f91 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -17352,11 +17352,11 @@  rs6000_expand_atomic_compare_and_swap (rtx operands[])
       retval = gen_reg_rtx (SImode);
       mode = SImode;
     }
+  else if (reg_overlap_mentioned_p (retval, oldval))
+    oldval = copy_to_reg (oldval);
 
   rs6000_pre_atomic_barrier (mod_s);
 
-  emit_move_insn (boolval, const0_rtx);
-
   label1 = NULL_RTX;
   if (!is_weak)
     {
@@ -17374,28 +17374,23 @@  rs6000_expand_atomic_compare_and_swap (rtx operands[])
 			       NULL_RTX, 1, OPTAB_LIB_WIDEN);
     }
 
-  x = gen_rtx_NE (VOIDmode, x, oldval);
-  x = rs6000_generate_compare (x, mode);
+  cond = gen_reg_rtx (CCmode);
+  x = gen_rtx_COMPARE (CCmode, x, oldval);
+  emit_insn (gen_rtx_SET (VOIDmode, cond, x));
+
+  x = gen_rtx_NE (VOIDmode, cond, const0_rtx);
   emit_unlikely_jump (x, label2);
 
   x = newval;
   if (mask)
     x = rs6000_mask_atomic_subword (retval, newval, mask);
 
-  cond = gen_reg_rtx (CCmode);
   emit_store_conditional (mode, cond, mem, x);
 
-  if (is_weak)
-    {
-      /* ??? It's either this or an unlikely jump over (set bool 1).  */
-      x = gen_rtx_EQ (SImode, cond, const0_rtx);
-      emit_insn (gen_rtx_SET (VOIDmode, boolval, x));
-    }
-  else
+  if (!is_weak)
     {
       x = gen_rtx_NE (VOIDmode, cond, const0_rtx);
       emit_unlikely_jump (x, label1);
-      emit_move_insn (boolval, const1_rtx);
     }
 
   if (mod_f != MEMMODEL_RELAXED)
@@ -17408,6 +17403,10 @@  rs6000_expand_atomic_compare_and_swap (rtx operands[])
 
   if (shift)
     rs6000_finish_atomic_subword (operands[1], retval, shift);
+
+  /* In all cases, CR0 contains EQ on success, and NE on failure.  */
+  x = gen_rtx_EQ (SImode, cond, const0_rtx);
+  emit_insn (gen_rtx_SET (VOIDmode, boolval, x));
 }
 
 /* Expand an atomic exchange operation.  */