[Bug other/55981] New: std::atomic store is split in two smaller stores

[Bug other/55981] New: std::atomic store is split in two smaller stores

[eugeni.stepanov at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55981

             Bug #: 55981
           Summary: std::atomic store is split in two smaller stores
    Classification: Unclassified
           Product: gcc
           Version: 4.7.2
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: other
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: eugeni.stepanov@gmail.com


Created attachment 29166
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29166
miscompiled program

Hi,

in the attached program, std::atomic<unsigned long> store is compiled as two
4-byte stores, becoming not atomic at all. This happens at -O2 and higher.

To reproduce,
# g++ -std=c++11 -O3 1.cc  -lpthread -o 1
# ./1
100000004

^ this can also be 300000002, both outcomes are invalid.

[Bug other/55981] std::atomic store is split in two smaller stores

[eugeni.stepanov at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55981

--- Comment #1 from Evgeniy Stepanov <eugeni.stepanov at gmail dot com> 2013-01-14 22:45:30 UTC ---
Disassembly of the loop in ff() function:

  4007c0:       movl   $0x2,0x2008de(%rip)        # 6010a8 <y>
  4007ca:       movl   $0x1,0x2008d8(%rip)        # 6010ac <y+0x4>
  4007d4:       sub    $0x1,%eax
  4007d7:       mfence 
  4007da:       movl   $0x4,0x2008c4(%rip)        # 6010a8 <y>
  4007e4:       movl   $0x3,0x2008be(%rip)        # 6010ac <y+0x4>
  4007ee:       mfence 
  4007f1:       jne    4007c0 <_Z2ffv+0xb0>

Does not look atomic.
Has all the proper fences, though.

[Bug other/55981] std::atomic store is split in two smaller stores

[eugeni.stepanov at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55981

--- Comment #2 from Evgeniy Stepanov <eugeni.stepanov at gmail dot com> 2013-01-14 22:48:08 UTC ---
Btw, the same happens if atomic is replaced with "volatile unsigned long y" -
which does not violate the standard, but may be considered undesirable by some.
I don't have a strong opinion about this.

[Bug other/55981] std::atomic store is split in two smaller stores

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55981

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Target|                            |i?86-*-* x86_64-*-*
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-01-14
     Ever Confirmed|0                           |1

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> 2013-01-14 23:08:10 UTC ---
The tree level is correct:
  __atomic_store_8 (&MEM[(struct __atomic_base *)&y]._M_i, 4294967298, 5);
  __atomic_store_8 (&MEM[(struct __atomic_base *)&y]._M_i, 12884901892, 5);
Expansion is correct:
;; __atomic_store_8 (&MEM[(struct __atomic_base *)&y]._M_i, 4294967298, 5);

(insn 31 30 32 (set (reg:DI 84)
        (const_int 4294967298 [0x100000002]))
/data1/src/gcc-cavium/toolchain/x86_64-tools/bin/../lib/gcc/x86_64-montavista-linux-gnu/4.7.0/../../../../x86_64-montavista-linux-gnu/include/c++/4.7.0/bits/atomic_base.h:438
-1
     (nil))

(insn 32 31 33 (set (mem/v:DI (symbol_ref:DI ("y") [flags 0x2]  <var_decl
0x7f5cdac4c0a0 y>) [-1 S8 A64])
        (reg:DI 84))
/data1/src/gcc-cavium/toolchain/x86_64-tools/bin/../lib/gcc/x86_64-montavista-linux-gnu/4.7.0/../../../../x86_64-montavista-linux-gnu/include/c++/4.7.0/bits/atomic_base.h:438
-1
     (nil))

(insn 33 32 0 (set (mem/v:BLK (scratch:DI) [0 A8])
        (unspec:BLK [
                (mem/v:BLK (scratch:DI) [0 A8])
            ] UNSPEC_MFENCE))
/data1/src/gcc-cavium/toolchain/x86_64-tools/bin/../lib/gcc/x86_64-montavista-linux-gnu/4.7.0/../../../../x86_64-montavista-linux-gnu/include/c++/4.7.0/bits/atomic_base.h:438
-1
     (nil))

cprop3 does (though there might be cost issues):
(insn 66 30 33 3 (set (mem/v:DI (symbol_ref:DI ("y") [flags 0x2]  <var_decl
0x7f5cdac4c0a0 y>) [-1 S8 A64])
        (const_int 4294967298 [0x100000002]))
/data1/src/gcc-cavium/toolchain/x86_64-tools/bin/../lib/gcc/x86_64-montavista-linux-gnu/4.7.0/../../../../x86_64-montavista-linux-gnu/include/c++/4.7.0/bits/atomic_base.h:438
62 {*movdi_internal_rex64}
     (nil))

(insn 33 66 35 3 (set (mem/v:BLK (scratch:DI) [0 A8])
        (unspec:BLK [
                (mem/v:BLK (scratch:DI) [0 A8])
            ] UNSPEC_MFENCE))
/data1/src/gcc-cavium/toolchain/x86_64-tools/bin/../lib/gcc/x86_64-montavista-linux-gnu/4.7.0/../../../../x86_64-montavista-linux-gnu/include/c++/4.7.0/bits/atomic_base.h:438
2052 {mfence_sse2}
     (nil))

peephole2 then comes around and does the splitting of the atomic instruction so
this is a target issue.

[Bug target/55981] std::atomic store is split in two smaller stores

[paulmck at linux dot vnet.ibm.com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55981

Paul E. McKenney <paulmck at linux dot vnet.ibm.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |paulmck at linux dot
                   |                            |vnet.ibm.com

--- Comment #4 from Paul E. McKenney <paulmck at linux dot vnet.ibm.com> 2013-01-15 12:30:04 UTC ---
(In reply to comment #2)
> Btw, the same happens if atomic is replaced with "volatile unsigned long y" -
> which does not violate the standard, but may be considered undesirable by some.
> I don't have a strong opinion about this.

This really does need to be fixed in the "volatile unsigned long y" case, or
device drivers storing constants to device registers will break.  So please fix
this for volatiles as well as atomics.

[Bug target/55981] std::atomic store is split in two smaller stores

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55981

--- Comment #5 from Uros Bizjak <ubizjak at gmail dot com> 2013-01-15 14:22:14 UTC ---
Obviously, not all DImode immediates are atomic. I have a patch that prohibits
moves of immediates that do not satisfy "e" constraints to volatile memories.

[Bug target/55981] std::atomic store is split in two smaller stores

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55981

--- Comment #6 from Uros Bizjak <ubizjak at gmail dot com> 2013-01-15 14:25:33 UTC ---
Created attachment 29169
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29169
Patch that prevents non-atomic immediates in moves to volatile memory location

[Bug target/55981] std::atomic store is split in two smaller stores

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55981

Uros Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|unassigned at gcc dot       |ubizjak at gmail dot com
                   |gnu.org                     |
   Target Milestone|---                         |4.7.3

--- Comment #7 from Uros Bizjak <ubizjak at gmail dot com> 2013-01-15 14:29:44 UTC ---
Attached patch generates:

        movabsq $4294967298, %rdx
        subl    $1, %eax
        movq    %rdx, y(%rip)
        movabsq $12884901892, %rdx
        mfence
        movq    %rdx, y(%rip)
        mfence

while non-patched compiler generates:

        movl    $2, y(%rip)
        movl    $1, y+4(%rip)
        subl    $1, %eax
        mfence
        movl    $4, y(%rip)
        movl    $3, y+4(%rip)
        mfence

[Bug target/55981] std::atomic store is split in two smaller stores

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55981

--- Comment #8 from Uros Bizjak <ubizjak at gmail dot com> 2013-01-15 19:37:55 UTC ---
Actually, the patch from comment(In reply to comment #6)
> Created attachment 29169 [details]
> Patch that prevents non-atomic immediates in moves to volatile memory location

This patch doesn't actually work. Floating point moves split to DImode move
pattern, and they assume that all immediates can be passed.

Original test can be fixed on 4.8 by following patch:

--cut here--
Index: sync.md
===================================================================
--- sync.md     (revision 195207)
+++ sync.md     (working copy)
@@ -225,11 +225,8 @@
        }

       /* Otherwise use a store.  */
-      if (INTVAL (operands[2]) & IX86_HLE_RELEASE)
-       emit_insn (gen_atomic_store<mode>_1 (operands[0], operands[1],
-                                            operands[2]));
-      else
-       emit_move_insn (operands[0], operands[1]);
+      emit_insn (gen_atomic_store<mode>_1 (operands[0], operands[1],
+                                          operands[2]));
     }
   /* ... followed by an MFENCE, if required.  */
   if (model == MEMMODEL_SEQ_CST)
--cut here--

[Bug target/55981] std::atomic store is split in two smaller stores

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55981

--- Comment #9 from Uros Bizjak <ubizjak at gmail dot com> 2013-01-15 19:39:20 UTC ---
(In reply to comment #4)
> (In reply to comment #2)
> > Btw, the same happens if atomic is replaced with "volatile unsigned long y" -
> > which does not violate the standard, but may be considered undesirable by some.
> > I don't have a strong opinion about this.
> 
> This really does need to be fixed in the "volatile unsigned long y" case, or
> device drivers storing constants to device registers will break.  So please fix
> this for volatiles as well as atomics.

Please open a new PR for this.

[Bug target/55981] std::atomic store is split in two smaller stores

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55981

--- Comment #10 from Uros Bizjak <ubizjak at gmail dot com> 2013-01-15 20:01:41 UTC ---
(In reply to comment #9)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > Btw, the same happens if atomic is replaced with "volatile unsigned long y" -
> > > which does not violate the standard, but may be considered undesirable by some.
> > > I don't have a strong opinion about this.
> > 
> > This really does need to be fixed in the "volatile unsigned long y" case, or
> > device drivers storing constants to device registers will break.  So please fix
> > this for volatiles as well as atomics.
> 
> Please open a new PR for this.

It looks to me that volatiles should not be allowed as offsetable operands, but
let's discuss this in a separate PR.

[Bug target/55981] std::atomic store is split in two smaller stores

[uros at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55981

--- Comment #11 from uros at gcc dot gnu.org 2013-01-17 16:25:04 UTC ---
Author: uros
Date: Thu Jan 17 16:24:54 2013
New Revision: 195273

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=195273
Log:
    PR target/55981
    * config/i386/sync.md (atomic_store<mode>): Always generate SWImode
    store through atomic_store<mode>_1.
    (atomic_store<mode>_1): Macroize insn using SWI mode iterator.

testsuite/ChangeLog:

    PR target/55981
    * gcc.target/pr55981.c: New test.


Added:
    trunk/gcc/testsuite/gcc.target/i386/pr55981.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/sync.md
    trunk/gcc/testsuite/ChangeLog

[Bug target/55981] std::atomic store is split in two smaller stores

[uros at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55981

--- Comment #12 from uros at gcc dot gnu.org 2013-01-17 22:51:07 UTC ---
Author: uros
Date: Thu Jan 17 22:51:00 2013
New Revision: 195283

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=195283
Log:
    Backport from mainline
    2012-01-17  Uros Bizjak  <ubizjak@gmail.com>

    PR target/55981
    * config/i386/sync.md (atomic_store<mode>): Generate SWImode
    store through atomic_store<mode>_1.
    (atomic_store<mode>_1): Macroize insn using SWI mode iterator.

testsuite/ChangeLog:

    Backport from mainline
    2012-01-17  Uros Bizjak  <ubizjak@gmail.com>

    PR target/55981
    * gcc.target/pr55981.c: New test.


Added:
    branches/gcc-4_7-branch/gcc/testsuite/gcc.target/pr55981.c
Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/config/i386/sync.md
    branches/gcc-4_7-branch/gcc/testsuite/ChangeLog

[Bug target/55981] std::atomic store is split in two smaller stores

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55981

Uros Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
                URL|                            |http://gcc.gnu.org/ml/gcc-p
                   |                            |atches/2013-01/msg00870.htm
                   |                            |l
         Resolution|                            |FIXED

--- Comment #13 from Uros Bizjak <ubizjak at gmail dot com> 2013-01-17 22:53:58 UTC ---
Fixed.

[Bug target/55981] std::atomic store is split in two smaller stores

[jackie.rosen at hushmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55981

Jackie Rosen <jackie.rosen at hushmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jackie.rosen at hushmail dot com

--- Comment #14 from Jackie Rosen <jackie.rosen at hushmail dot com> ---
*** Bug 260998 has been marked as a duplicate of this bug. ***
Seen from the domain http://volichat.com
Page where seen: http://volichat.com/adult-chat-rooms
Marked for reference. Resolved as fixed @bugzilla.