[Bug target/56028] New: Splitting a 64-bit volatile store

[Bug target/56028] New: Splitting a 64-bit volatile store

[amonakov at gcc dot gnu.org]

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

             Bug #: 56028
           Summary: Splitting a 64-bit volatile store
    Classification: Unclassified
           Product: gcc
           Version: 4.7.2
               URL: http://gcc.gnu.org/ml/gcc-patches/2013-01/msg00870.htm
                    l
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: amonakov@gcc.gnu.org
                CC: eugeni.stepanov@gmail.com, khlebnikov@openvz.org,
                    mkl@pengutronix.de, paulmck@linux.vnet.ibm.com,
                    ubizjak@gmail.com, valery.bickov+gcc@gmail.com
            Target: i?86-*-* x86_64-*-*


Created attachment 29201
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29201
test case

+++ This bug was initially created as a clone of Bug #55981 +++

As suggested in the parent bug, this is a separate report dealing specifically
with splitting of a 64-bit volatile (not necessarily atomic) store of an
immediate value into 32-bit accesses.  Comment 10 of the original report says:

> > > 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/56028] Splitting a 64-bit volatile store

[ubizjak at gmail dot com]

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

--- Comment #1 from Uros Bizjak <ubizjak at gmail dot com> 2013-01-18 11:09:42 UTC ---
- Does language standard guarantee atomic store in this case [wikipedia says
"No." [1]]?

- Can a store to a volatile DImode location be implemented as two consecutive
SImode stores to adjacent location (this breaks stores of 64bit immediates to
MMIO 64bit registers)?

[1] http://en.wikipedia.org/wiki/Volatile_variable#In_C_and_C.2B.2B

[Bug target/56028] Splitting a 64-bit volatile store

[paulmck at linux dot vnet.ibm.com]

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

--- Comment #2 from Paul E. McKenney <paulmck at linux dot vnet.ibm.com> 2013-01-18 11:25:52 UTC ---
(In reply to comment #1)
> - Does language standard guarantee atomic store in this case [wikipedia says
> "No." [1]]?

The above example of device drivers storing constants to a device register
declared as "volatile unsigned long y" does not require all of the attributes
of an atomic store, for example, it does not require memory-fence instructions.

> - Can a store to a volatile DImode location be implemented as two consecutive
> SImode stores to adjacent location (this breaks stores of 64bit immediates to
> MMIO 64bit registers)?

See 1.9p8 of the C++11 standard, first bullet:

"Access to volatile objects are evaluated strictly according to the rules of
the abstract machine."

>From what I can see, implementing a store to a volatile DImode location as two
consecutive SImode stores to adjacent locations violates this aspect of the
standard.  Furthermore, to expand on your parenthesized statement above, gcc
might not operate reliably if the device drivers in the kernel it is running on
have their 64-bit immediate stores broken into pairs of 32-bit immediate
stores.  ;-)

> [1] http://en.wikipedia.org/wiki/Volatile_variable#In_C_and_C.2B.2B

[Bug target/56028] Splitting a 64-bit volatile store

[eugeni.stepanov at gmail dot com]

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

--- Comment #3 from Evgeniy Stepanov <eugeni.stepanov at gmail dot com> 2013-01-18 11:57:07 UTC ---
(In reply to comment #2)
> See 1.9p8 of the C++11 standard, first bullet:
> 
> "Access to volatile objects are evaluated strictly according to the rules of
> the abstract machine."
> 
> From what I can see, implementing a store to a volatile DImode location as two
> consecutive SImode stores to adjacent locations violates this aspect of the
> standard.  Furthermore, to expand on your parenthesized statement above, gcc
> might not operate reliably if the device drivers in the kernel it is running on
> have their 64-bit immediate stores broken into pairs of 32-bit immediate
> stores.  ;-)

So, what are these "rules of the abstract machine", and why do they allow
non-atomic store of a large volatile aggregate (it is definitely not atomic,
right?), and require atomicity for volatile long?

[Bug target/56028] Splitting a 64-bit volatile store

[paulmck at linux dot vnet.ibm.com]

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

--- Comment #4 from Paul E. McKenney <paulmck at linux dot vnet.ibm.com> 2013-01-18 16:22:49 UTC ---
(In reply to comment #3)
> So, what are these "rules of the abstract machine", and why do they allow
> non-atomic store of a large volatile aggregate (it is definitely not atomic,
> right?), and require atomicity for volatile long?

5.17p3 and 5.17p4 do distinguish between non-aggregate and aggregate
assignment:

5.17p3:

        "If the left operand is not of class type, the expression is 
        implicitly converted (Clause 4) to the cv-unqualified type of
        the left operand."

5.17p4:

        "If the left operand is of class type, the class shall be 
        complete.  Assignment to objects of a class is defined by the
        copy/move assignment operator."

Of course, the old-days possibility of systems with 8-bit busses limits how
much the standard can say, but given that the system in question really can do
a 64-bit store, volatile really should force a single store.

[Bug target/56028] Splitting a 64-bit volatile store

[eugeni.stepanov at gmail dot com]

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

--- Comment #5 from Evgeniy Stepanov <eugeni.stepanov at gmail dot com> 2013-01-18 16:38:11 UTC ---
Well, it's true that classes have assignment operators, and basic types don't.
But this does not have anything to do with how the assignment could (or could
not) be implemented at the machine level.

AFAIU, the main point here is that a valid, data-race-free program can't
observe the non-atomicity of a 64-bit store. And there is no special clause for
volatile stores in this regard, either.

I agree that in practice volatile stores of reasonably-sized basic types must
be atomic, or a lot of legacy code will break.

[Bug target/56028] Splitting a 64-bit volatile store

[paulmck at linux dot vnet.ibm.com]

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

--- Comment #6 from Paul E. McKenney <paulmck at linux dot vnet.ibm.com> 2013-01-18 17:40:13 UTC ---
The fact that a data-race-free program cannot observe the non-atomicity of a
64-bit store, though true, is beside the point.  The plain fact is that
hardware registers (for which volatile was intended) really do care about the
size of the store.  A pair of 32-bit stores does not necessarily mean the same
thing to hardware as does a single 64-bit store.  Given that C is intended to
be used for device drivers, volatile stores of reasonably-sized basic types
must be atomic, and on 64-bit systems, "reasonably-sized" very clearly includes
64-bit stores.

So this bug really does need to be fixed.

[Bug target/56028] Splitting a 64-bit volatile store

[pluto at agmk dot net]

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

Pawel Sikora <pluto at agmk dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |pluto at agmk dot net

--- Comment #7 from Pawel Sikora <pluto at agmk dot net> 2013-01-18 18:23:27 UTC ---
(In reply to comment #6)
> The fact that a data-race-free program cannot observe the non-atomicity of a
> 64-bit store, though true, is beside the point.  The plain fact is that
> hardware registers (for which volatile was intended) really do care about the
> size of the store.  A pair of 32-bit stores does not necessarily mean the same
> thing to hardware as does a single 64-bit store.  Given that C is intended to
> be used for device drivers, volatile stores of reasonably-sized basic types
> must be atomic, and on 64-bit systems, "reasonably-sized" very clearly includes
> 64-bit stores.

even 'movq' on x86-64 could be split into 32-bit transfers by mainboard h/w.
devices connected to x86-64 should be aware of this!

btw, iirc the x87-fpu load/store can give you atomic 64-bit transfer.

[Bug target/56028] Splitting a 64-bit volatile store

[paulmck at linux dot vnet.ibm.com]

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

--- Comment #8 from Paul E. McKenney <paulmck at linux dot vnet.ibm.com> 2013-01-19 12:35:12 UTC ---
Indeed, different hardware implementations can cause all sorts of mischief. 
Nevertheless, the compiler should not also provide mischief in these cases.

In addition, as noted in comment 5, there is legacy software.  I understand and
completely support use of atomics when they are generally available (having
assisted in their definition), but many projects will need to work with older
compilers for quite some time to come.  Therefore, volatile loads, which are
only idiom available in these older compilers, needs to continue to provide the
needed functionality.  Again as noted in comment 5, volatile stores of
reasonably-sized basic types must be atomic.

[Bug target/56028] Splitting a 64-bit volatile store

[ubizjak at gmail dot com]

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

Uros Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
                URL|http://gcc.gnu.org/ml/gcc-p |
                   |atches/2013-01/msg00870.htm |
                   |l                           |
   Last reconfirmed|                            |2013-01-22
         AssignedTo|unassigned at gcc dot       |ubizjak at gmail dot com
                   |gnu.org                     |
     Ever Confirmed|0                           |1

--- Comment #9 from Uros Bizjak <ubizjak at gmail dot com> 2013-01-22 07:32:59 UTC ---
I'll look into this.

[Bug target/56028] Splitting a 64-bit volatile store

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-22 08:18:23 UTC ---
Can't we in the movdi expander detect writing to volatile MEM and expand it as
atomic_storedi with MEMMODEL_RELAXED instead?

[Bug target/56028] Splitting a 64-bit volatile store

[ubizjak at gmail dot com]

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

--- Comment #11 from Uros Bizjak <ubizjak at gmail dot com> 2013-01-22 08:46:48 UTC ---
I was thinking of removing (!o,n) alternative from movdi (together with
corresponding splitters). Splitter/peephole2 actually always generates movabs
$N,%reg; mov $reg,(mem) unless it can't get a register. In the later case, the
move is split into two separate moves.

The problem is, that other move patterns assume (i.e.
*mov{df,xf,ti}_internal_rex64) that they can move all immediates to memory.
When moving FP immediate to volatile DFmode location, gcc tries to use DImode
move with immediate operand, so this would fail with removed (!o,n)
alternative.

Also, split_double_mode, split_to_parts and ix86_split_long_move will have to
be reviewed. Please see the comment inside split_double_mode, how
simplify_subreg refuses to split volatile memory address, but gcc manages to
get around this "limitation".

[Bug target/56028] Splitting a 64-bit volatile store

[ubizjak at gmail dot com]

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

Uros Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |http://gcc.gnu.org/ml/gcc-p
                   |                            |atches/2013-01/msg01119.htm
                   |                            |l
   Target Milestone|---                         |4.7.3

--- Comment #12 from Uros Bizjak <ubizjak at gmail dot com> 2013-01-22 20:58:03 UTC ---
Patch at [1].

[1] http://gcc.gnu.org/ml/gcc-patches/2013-01/msg01119.html

[Bug target/56028] Splitting a 64-bit volatile store

[uros at gcc dot gnu.org]

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

--- Comment #13 from uros at gcc dot gnu.org 2013-01-22 20:58:45 UTC ---
Author: uros
Date: Tue Jan 22 20:58:37 2013
New Revision: 195386

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=195386
Log:
    PR target/56028
    * config/i386/i386.md (*movti_internal_rex64): Change (o,riF)
    alternative to (o,r).
    (*movdi_internal_rex64): Remove (!o,n) alternative.
    (DImode immediate->memory splitter): Remove.
    (DImode immediate->memory peephole2): Remove.
    (movtf): Enable for TARGET_64BIT || TARGET_SSE.
    (*movtf_internal_rex64): Rename from *movtf_internal. Change (!o,F*r)
    alternative to (!o,*r).
    (*movtf_internal_sse): New pattern.
    (*movxf_internal_rex64): New pattern.
    (*movxf_internal): Disable for TARGET_64BIT.
    (*movdf_internal_rex64): Remove (!o,F) alternative.

testsuite/ChangeLog:

2012-01-22  Uros Bizjak  <ubizjak@gmail.com>

    PR target/56028
    * gcc.target/i386/pr56028.c: New test.


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

[Bug target/56028] Splitting a 64-bit volatile store

[uros at gcc dot gnu.org]

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

--- Comment #14 from uros at gcc dot gnu.org 2013-01-27 14:28:23 UTC ---
Author: uros
Date: Sun Jan 27 14:28:19 2013
New Revision: 195495

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

    PR target/56028
    * config/i386/i386.md (*movti_internal_rex64): Change (o,riF)
    alternative to (o,r).
    (*movdi_internal_rex64): Remove (!o,n) alternative.
    (DImode immediate->memory splitter): Remove.
    (DImode immediate->memory peephole2): Remove.
    (movtf): Enable for TARGET_64BIT || TARGET_SSE.
    (*movtf_internal_rex64): Rename from *movtf_internal. Change (!o,F*r)
    alternative to (!o,*r).
    (*movtf_internal_sse): New pattern.
    (*movxf_internal_rex64): New pattern.
    (*movxf_internal): Disable for TARGET_64BIT.
    (*movdf_internal_rex64): Remove (!o,F) alternative.

    2013-01-23  Uros Bizjak  <ubizjak@gmail.com>

    * config/i386/i386.md (*movdf_internal_rex64): Disparage alternatives
    involving stack registers slightly.

    2013-01-24  Uros Bizjak  <ubizjak@gmail.com>

    * config/i386/constraints.md (Yf): New constraint.
    * config/i386/i386.md (*movdf_internal_rex64): Use Yf*f instead
    of f constraint to conditionaly disable x87 register preferences.
    (*movdf_internal): Ditto.
    (*movsf_internal): Ditto.

    2012-01-24  Uros Bizjak  <ubizjak@gmail.com>

    * config/i386/i386.md (*movti_internal_rex64): Add (o,e) alternative.
    (*movtf_internal_rex64): Add (!o,C) alternative
    (*movxf_internal_rex64): Ditto.
    (*movdf_internal_rex64): Add (?r,C) and (?m,C) alternatives.

testsuite/ChangeLog:

    Backport from mainline
    2013-01-22  Uros Bizjak  <ubizjak@gmail.com>

    PR target/56028
    * gcc.target/i386/pr56028.c: New test.

    2013-01-24  Uros Bizjak  <ubizjak@gmail.com>

    * gcc.target/i386/movsd.c: New test.


Added:
    branches/gcc-4_7-branch/gcc/testsuite/gcc.target/i386/movsd.c
    branches/gcc-4_7-branch/gcc/testsuite/gcc.target/i386/pr56028.c
Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/config/i386/constraints.md
    branches/gcc-4_7-branch/gcc/config/i386/i386.md
    branches/gcc-4_7-branch/gcc/testsuite/ChangeLog

[Bug target/56028] Splitting a 64-bit volatile store

[ubizjak at gmail dot com]

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

Uros Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #15 from Uros Bizjak <ubizjak at gmail dot com> 2013-01-27 14:30:04 UTC ---
Fixed.