| Message ID | CAFULd4Yna7o-BFv5CfyZQ6NnKMMyYAnO-FxnNBpOgk1LA=yo_g@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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~
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
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.
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~
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.
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~
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.
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
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
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" } } */