Patchwork [i386] : Implement atomic_fetch_sub

login
register
mail settings
Submitter Uros Bizjak
Date Aug. 3, 2012, 7:24 a.m.
Message ID <CAFULd4Yna7o-BFv5CfyZQ6NnKMMyYAnO-FxnNBpOgk1LA=yo_g@mail.gmail.com>
Download mbox | patch
Permalink /patch/174903/
State New
Headers show

Comments

Uros Bizjak - Aug. 3, 2012, 7:24 a.m.
On Fri, Aug 3, 2012 at 8:40 AM, Uros Bizjak <ubizjak@gmail.com> wrote:

> We can implement atomic_fetch_sub with atomic_fetch_add and inverted
> operand 2. However, we have to prevent overflows with negation, so
> only const_int operand 2 is allowed in the expander.
>
> 2012-08-02  Uros Bizjak  <ubizjak@gmail.com>
>
>         PR target/54087
>         * config/i386/sync.md (atomic_fetch_sub<mode>): New expander.
>
> Tested on x86_64-pc-linux-gnu {,-m32}.
>
> I will wait for a day for possible comments.

Patch attached again, this time with a testcase.

2012-08-02  Uros Bizjak  <ubizjak@gmail.com>

	PR target/54087
	* gcc.target/i386/pr54087.c: New test.

Uros.
Richard Guenther - Aug. 3, 2012, 8:02 a.m.
On Fri, Aug 3, 2012 at 9:24 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Fri, Aug 3, 2012 at 8:40 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>
>> We can implement atomic_fetch_sub with atomic_fetch_add and inverted
>> operand 2. However, we have to prevent overflows with negation, so
>> only const_int operand 2 is allowed in the expander.

Why do you need to prevent overflows?

Richard.

>> 2012-08-02  Uros Bizjak  <ubizjak@gmail.com>
>>
>>         PR target/54087
>>         * config/i386/sync.md (atomic_fetch_sub<mode>): New expander.
>>
>> Tested on x86_64-pc-linux-gnu {,-m32}.
>>
>> I will wait for a day for possible comments.
>
> Patch attached again, this time with a testcase.
>
> 2012-08-02  Uros Bizjak  <ubizjak@gmail.com>
>
>         PR target/54087
>         * gcc.target/i386/pr54087.c: New test.
>
> Uros.
Uros Bizjak - Aug. 3, 2012, 8:51 a.m.
On Fri, Aug 3, 2012 at 10:02 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Fri, Aug 3, 2012 at 9:24 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Fri, Aug 3, 2012 at 8:40 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>>
>>> We can implement atomic_fetch_sub with atomic_fetch_add and inverted
>>> operand 2. However, we have to prevent overflows with negation, so
>>> only const_int operand 2 is allowed in the expander.
>
> Why do you need to prevent overflows?

Because we change (x)sub to (x)add, so for SImode operation - without
overflow check -

sub 0x80000000, %eax

gets changed to

add 0x80000000, %eax.

The same reasoning goes for dynamic negation: for neg %eax,%eax value
0x80000000 stays the same, but we have changed (x)sub to an (x)add in
the code stream.

Uros.
Richard Henderson - Aug. 3, 2012, 2:40 p.m.
On 2012-08-03 01:51, Uros Bizjak wrote:
> The same reasoning goes for dynamic negation: for neg %eax,%eax value
> 0x80000000 stays the same, but we have changed (x)sub to an (x)add in
> the code stream.

So?  Did you think the xadd will trap?


r~
Andrew MacLeod - Aug. 3, 2012, 3:01 p.m.
On 08/03/2012 10:40 AM, Richard Henderson wrote:
> On 2012-08-03 01:51, Uros Bizjak wrote:
>> The same reasoning goes for dynamic negation: for neg %eax,%eax value
>> 0x80000000 stays the same, but we have changed (x)sub to an (x)add in
>> the code stream.
> So?  Did you think the xadd will trap?
>
>
Is there a good reason we can't do this in the generic code in  
optabs.c::expand_atomic_fetch_op()?

so if there is no ADD or SUB pattern, and the reverse does exist,  we 
just negate the operand and use the opposite pattern...

we already use the reverse code for 'fixup' code if for instance 
fetch_after exists and fetch_before doesn't.  Seems like a pretty 
trivial extension.

Andrew
Uros Bizjak - Aug. 3, 2012, 3:01 p.m.
On Fri, Aug 3, 2012 at 4:40 PM, Richard Henderson <rth@redhat.com> wrote:
> On 2012-08-03 01:51, Uros Bizjak wrote:
>> The same reasoning goes for dynamic negation: for neg %eax,%eax value
>> 0x80000000 stays the same, but we have changed (x)sub to an (x)add in
>> the code stream.
>
> So?  Did you think the xadd will trap?

No, but can we ignore the fact that we changed xsub -0x80000000, mem
to xadd -0x080000000, mem?

Uros.
Richard Henderson - Aug. 3, 2012, 3:08 p.m.
On 2012-08-03 08:01, Uros Bizjak wrote:
> On Fri, Aug 3, 2012 at 4:40 PM, Richard Henderson <rth@redhat.com> wrote:
>> On 2012-08-03 01:51, Uros Bizjak wrote:
>>> The same reasoning goes for dynamic negation: for neg %eax,%eax value
>>> 0x80000000 stays the same, but we have changed (x)sub to an (x)add in
>>> the code stream.
>>
>> So?  Did you think the xadd will trap?
> 
> No, but can we ignore the fact that we changed xsub -0x80000000, mem
> to xadd -0x080000000, mem?

Yes, since it'll have the same effect on the bits.


r~
Uros Bizjak - Aug. 3, 2012, 6:02 p.m.
On Fri, Aug 3, 2012 at 5:08 PM, Richard Henderson <rth@redhat.com> wrote:

>>>> The same reasoning goes for dynamic negation: for neg %eax,%eax value
>>>> 0x80000000 stays the same, but we have changed (x)sub to an (x)add in
>>>> the code stream.
>>>
>>> So?  Did you think the xadd will trap?
>>
>> No, but can we ignore the fact that we changed xsub -0x80000000, mem
>> to xadd -0x080000000, mem?
>
> Yes, since it'll have the same effect on the bits.

Nice, so we can go with full

--cut here--
;; We can implement atomic_fetch_sub as
;; atomic_fetch_add with negated operand 2.
(define_expand "atomic_fetch_sub<mode>"
  [(set (match_operand:SWI 0 "register_operand")
        (unspec_volatile:SWI
          [(match_operand:SWI 1 "memory_operand")
           (match_operand:SI 3 "const_int_operand")]            ;; model
          UNSPECV_XCHG))
   (set (match_dup 1)
        (minus:SWI (match_dup 1)
                   (match_operand:SWI 2 "nonmemory_operand")))
   (clobber (reg:CC FLAGS_REG))]
  "TARGET_XADD"
{
  emit_insn
   (gen_atomic_fetch_add<mode> (operands[0], operands[1],
                                negate_rtx (<MODE>mode, operands[2]),
                                operands[3]));
  DONE;
})
--cut here--

?

Uros.
Richard Henderson - Aug. 3, 2012, 6:11 p.m.
On 2012-08-03 11:02, Uros Bizjak wrote:
> Nice, so we can go with full [snip]

Unless we take Andrew's suggestion to simply do this in optabs.c instead.
Which is probably better.


r~
Uros Bizjak - Aug. 3, 2012, 6:14 p.m.
On Fri, Aug 3, 2012 at 8:11 PM, Richard Henderson <rth@redhat.com> wrote:
> On 2012-08-03 11:02, Uros Bizjak wrote:
>> Nice, so we can go with full [snip]
>
> Unless we take Andrew's suggestion to simply do this in optabs.c instead.
> Which is probably better.

Indeed. However, I'm not that familiar with this part of the compiler,
so I'll leave the implementation to someone else.

Thanks,
Uros.
Andrew MacLeod - Aug. 3, 2012, 6:52 p.m.
On 08/03/2012 02:14 PM, Uros Bizjak wrote:
> On Fri, Aug 3, 2012 at 8:11 PM, Richard Henderson<rth@redhat.com>  wrote:
>> On 2012-08-03 11:02, Uros Bizjak wrote:
>>> Nice, so we can go with full [snip]
>> Unless we take Andrew's suggestion to simply do this in optabs.c instead.
>> Which is probably better.
> Indeed. However, I'm not that familiar with this part of the compiler,
> so I'll leave the implementation to someone else.
>
>
I'll take a look at it next week unless someone gets to it first.

Andrew
Paolo Bonzini - Oct. 1, 2012, 5:21 p.m.
Il 03/08/2012 17:08, Richard Henderson ha scritto:
> On 2012-08-03 08:01, Uros Bizjak wrote:
>> On Fri, Aug 3, 2012 at 4:40 PM, Richard Henderson <rth@redhat.com> wrote:
>>> On 2012-08-03 01:51, Uros Bizjak wrote:
>>>> The same reasoning goes for dynamic negation: for neg %eax,%eax value
>>>> 0x80000000 stays the same, but we have changed (x)sub to an (x)add in
>>>> the code stream.
>>>
>>> So?  Did you think the xadd will trap?
>>
>> No, but can we ignore the fact that we changed xsub -0x80000000, mem
>> to xadd -0x080000000, mem?
> 
> Yes, since it'll have the same effect on the bits.

In fact we can even use this trick for "xxor"...

Paolo

Patch

Index: config/i386/sync.md
===================================================================
--- config/i386/sync.md	(revision 190111)
+++ config/i386/sync.md	(working copy)
@@ -520,6 +520,31 @@ 
   return "lock{%;} add{<imodesuffix>}\t{%1, %0|%0, %1}";
 })
 
+;; We can use atomic_fetch_add with negated operand 2 to implement
+;; atomic_fetch_sub.  We have to prevent overflows with negation, so
+;; only const_int operand 2 is allowed.
+(define_expand "atomic_fetch_sub<mode>"
+  [(set (match_operand:SWI 0 "register_operand")
+	(unspec_volatile:SWI
+	  [(match_operand:SWI 1 "memory_operand")
+	   (match_operand:SI 3 "const_int_operand")]		;; model
+	  UNSPECV_XCHG))
+   (set (match_dup 1)
+	(minus:SWI (match_dup 1)
+		   (match_operand:SWI 2 "const_int_operand")))
+   (clobber (reg:CC FLAGS_REG))]
+  "TARGET_XADD"
+{
+  /* Avoid overflows.  */
+  if (mode_signbit_p (<MODE>mode, operands[2]))
+    FAIL;
+
+  emit_insn (gen_atomic_fetch_add<mode> (operands[0], operands[1],
+					 negate_rtx (<MODE>mode, operands[2]),
+					 operands[3]));
+  DONE;
+})
+
 ;; Recall that xchg implicitly sets LOCK#, so adding it again wastes space.
 ;; In addition, it is always a full barrier, so we can ignore the memory model.
 (define_insn "atomic_exchange<mode>"
Index: testsuite/gcc.target/i386/pr54087.c
===================================================================
--- testsuite/gcc.target/i386/pr54087.c	(revision 0)
+++ testsuite/gcc.target/i386/pr54087.c	(revision 0)
@@ -0,0 +1,13 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+/* { dg-additional-options "-march=i486" { target ia32 } } */
+
+int v;
+
+int foo (void)
+{
+  return __atomic_sub_fetch (&v, 5, __ATOMIC_SEQ_CST);
+}
+
+/* { dg-final { scan-assembler "xadd" } } */
+/* { dg-final { scan-assembler-not "cmpxchg" } } */