[Bug target/110592] New: [SPARC] GCC should default to TSO memory model when compiling for sparc32

[Bug target/110592] New: [SPARC] GCC should default to TSO memory model when compiling for sparc32

[koachan+gccbugs at protonmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110592

            Bug ID: 110592
           Summary: [SPARC] GCC should default to TSO memory model when
                    compiling for sparc32
           Product: gcc
           Version: 12.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: koachan+gccbugs at protonmail dot com
  Target Milestone: ---

Created attachment 55501
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55501&action=edit
Reproducer of unwanted memory reordering under TSO processors

Currently, when targeting sparc32 processors, GCC assumes that the hardware has
sequentially consistent memory ordering by default.
This can cause problems when running generated binaries on v8 and later
processors, which uses weaker TSO ordering.

In the attached reproducer, when compiled with the default sparc32 target, the
resulting code is missing the required barriers:

00000fb0 <thread1Func()>:
...
    104c:       e0 26 00 00     st  %l0, [ %i0 ]
    1050:       c2 06 40 00     ld  [ %i1 ], %g1
    1054:       c2 26 80 00     st  %g1, [ %i2 ]
...

0000108c <thread2Func()>:
...
    1128:       e0 26 00 00     st  %l0, [ %i0 ]
    112c:       c2 06 40 00     ld  [ %i1 ], %g1
    1130:       c2 26 80 00     st  %g1, [ %i2 ]
...

Compare with the result when explicitly specifying -mcpu=v8:

00000fa4 <thread1Func()>:
...
    1040:       e0 26 00 00     st  %l0, [ %i0 ]
    1044:       c0 6b bf ff     ldstub  [ %sp + -1 ], %g0
    1048:       c0 6b bf ff     ldstub  [ %sp + -1 ], %g0
    104c:       c2 06 40 00     ld  [ %i1 ], %g1
    1050:       c2 26 80 00     st  %g1, [ %i2 ]
    1054:       c0 6b bf ff     ldstub  [ %sp + -1 ], %g0
...

0000108c <thread2Func()>:
...
    1128:       e0 26 00 00     st  %l0, [ %i0 ]
    112c:       c0 6b bf ff     ldstub  [ %sp + -1 ], %g0
    1130:       c0 6b bf ff     ldstub  [ %sp + -1 ], %g0
    1134:       c2 06 40 00     ld  [ %i1 ], %g1
    1138:       c2 26 80 00     st  %g1, [ %i2 ]
    113c:       c0 6b bf ff     ldstub  [ %sp + -1 ], %g0
...

This causes the default-target code to hit the assert condition.

Since all code that works on TSO processors will work on processors with a
stronger memory model (i.e sequential consistency), it is probably better if
GCC uses TSO by default unless otherwise specified (e.g by explicitly using
-mcpu=v7).

[Bug target/110592] [SPARC] GCC should default to TSO memory model when compiling for sparc32

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110592

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID
                 CC|                            |ebotcazou at gcc dot gnu.org

--- Comment #1 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
We're not going to change that now; compile for V8 if you want to run on V8.

[Bug target/110592] [SPARC] GCC should default to TSO memory model when compiling for sparc32

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110592

--- Comment #2 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
Note that the SPARC compiler can be configured --with-cpu or --with-cpu_32 or
--with-cpu_64 (as well as s/cpu/tune/) so you can locally tweak defaults.

[Bug target/110592] [SPARC] GCC should default to TSO memory model when compiling for sparc32

[martin at netbsd dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110592

Martin Husemann <martin at netbsd dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |martin at netbsd dot org

--- Comment #3 from Martin Husemann <martin at netbsd dot org> ---
Note that this makes it impossible for distributions to default to sparc v7
with an unmodified gcc.

Only options are
 - locally patch (fix) gcc
 - provide separate distribution binaries for v7 and v8
 - drop support for v7 completely

... which all three sound bad to me.

[Bug target/110592] [SPARC] GCC should default to TSO memory model when compiling for sparc32

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110592

--- Comment #4 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> Note that this makes it impossible for distributions to default to sparc v7
> with an unmodified gcc.
> 
> Only options are
>  - locally patch (fix) gcc
>  - provide separate distribution binaries for v7 and v8
>  - drop support for v7 completely
> 
> ... which all three sound bad to me.

Well, you need to elaborate a bit here, because the current configuration has
been there for a quarter of century and everybody had apparently survived it
until a couple of days ago.

[Bug target/110592] [SPARC] GCC should default to TSO memory model when compiling for sparc32

[campbell+gcc-bugzilla at mumble dot net]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110592

Taylor R Campbell <campbell+gcc-bugzilla at mumble dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |campbell+gcc-bugzilla@mumbl
                   |                            |e.net

--- Comment #5 from Taylor R Campbell <campbell+gcc-bugzilla at mumble dot net> ---
(In reply to Eric Botcazou from comment #4)
> Well, you need to elaborate a bit here, because the current configuration
> has been there for a quarter of century and everybody had apparently
> survived it until a couple of days ago.

For most of that quarter century, memory ordering was limited to out-of-line
barrier/fence subroutines implemented in assembly, like membar_sync in Solaris
and NetBSD, or the thread-switch assembly routines in the kernel.

It is only relatively recently, since C11 and C++11, that a lot of programs
started using in-line barriers/fences and ordered memory operations like
store-release/load-acquire.

In that time, sparcv7 and sparcv8 haven't gotten a lot of attention, of course.

But since they were introduced, NetBSD has had a common userland for sparcv7
and sparcv8, just called `NetBSD/sparc', with a special libc loaded on sparcv8
to use v8-only instructions like SMUL and UMUL for runtime multiplication
subroutines to improve performance.  (We could in principle do the same for
LDSTUB in membar_sync on sparcv7, although we don't at the moment.)

But now that programs rely on compiler-generated barriers, there's a conflict
between gcc's v7 and v8 code generation:

1. `gcc -mcpu=v7' generates code that lacks LDSTUB where store-before-load
barriers are needed, so anything that uses Dekker's algorithm with in-line
barriers won't work correctly on a sparcv8 CPU (but it will only manifest in
extremely rare, hard-to-diagnose scenarios, because Dekker's algorithm is so
obscure).

2. `gcc -mcpu=v8' generates code that uses SMUL and UMUL and other instructions
that don't exist on sparcv7.

Evidently gcc can be made to generate SMUL/UMUL but omit LDSTUB barriers by
using `gcc -mcpu=v8 -mmemory-model=sc', but the other way around doesn't work:
`gcc -mcpu=v7 -mmemory-model=tso' still omits the LDSTUB barriers, because the
code generation rules for barriers are all gated on TARGET_V8 || TARGET_V9.

What we would like to do for NetBSD/sparc is use `-mcpu=v7 -mmemory-model=tso'
-- that is, if it worked -- by default.  The original submitter drafted a
relatively small patch to achieve this, mostly by removing TARGET_V8 ||
TARGET_V9 conditionals or changing TARGET_V8 to !TARGET_V9 in membar-related
code generation rules.  But we'd also like to avoid diverging from gcc
upstream.  Could we convince you to take up an approach like this?

Applications built to run on v7-only, of course, could omit the LDSTUBs by
using `-mcpu=v7 -mmemory-model=sc' (or perhaps we could have the default be
`-mcpu=v7 -mmemory-model=sc', but have bare `-mcpu=v7' imply `-mcpu=v7
-mmemory-model=sc' or something), and applications built to run on v8-only can
still use `-mcpu=v8' to take advantage of `SMUL/UMUL'.

I expect this would only affect a tiny fraction of programs in extremely rare
scenarios -- those that actually rely on Dekker's algorithm (already rare), and
hit problems with memory ordering (also rare, only under high contention),
using in-line barriers or ordered memory operations (which wasn't the norm a
quarter century ago when v7 and v8 were relevant).  So you have to go out of
your way to hit problems in practice, and any negative performance impact of
the extra LDSTUBs on v7 CPUs that don't need them is likely negligible.  But
it's clear from code inspection and theory that the problem is there.

[Bug target/110592] [SPARC] GCC should default to TSO memory model when compiling for sparc32

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110592

--- Comment #6 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
Sorry, no, NetBSD/sparc is too obscure a platform to justify changing the
default for the entire compiler.  But you can do like Linux & Solaris and add
sparc/tso.h to the tm_file list of sparc-*-netbsdelf*) in config.gcc.

[Bug target/110592] [SPARC] GCC should default to TSO memory model when compiling for sparc32

[campbell+gcc-bugzilla at mumble dot net]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110592

--- Comment #7 from Taylor R Campbell <campbell+gcc-bugzilla at mumble dot net> ---
> Sorry, no, NetBSD/sparc is too obscure a platform to justify changing the
> default for the entire compiler.  But you can do like Linux & Solaris and
> add sparc/tso.h to the tm_file list of sparc-*-netbsdelf*) in config.gcc.

I don't understand, how would that help?  As I understand it, whenever
`-mcpu=v7', the memory model is just ignored -- even if we set it to TSO --
because all rules that depend on it are gated on TARGET_V8 || TARGET_V9 or
similar.

I'm not asking for you to change defaults in Linux or Solaris -- I'm just
asking to be _able_ to say `-mcpu=v7 -mmemory-model=tso' and get v7-only
instruction streams with the LDSTUBs needed for TSO.  Right now, with
`-mcpu=v7', passing `-mmemory-model=tso' has no effect.

[Bug target/110592] [SPARC] GCC should default to TSO memory model when compiling for sparc32

[koachan+gccbugs at protonmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110592

--- Comment #8 from Koakuma <koachan+gccbugs at protonmail dot com> ---
Created attachment 55529
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55529&action=edit
Proposed patch for relaxing the guards of barrier emission

Hello, sorry that I only got to reply now.
And yeah, I first noticed it when I was trying out some C++ concurrency
tutorials. A bit weird, I admit...

That being said,
> Sorry, no, NetBSD/sparc is too obscure a platform to justify changing the default for the entire compiler.
Understood. So the memory model default should not change, that is okay.
(And I believe the NetBSD folks also agree with me on this?)

However...

> But you can do like Linux & Solaris and add sparc/tso.h to the tm_file list of sparc-*-netbsdelf*) in config.gcc.
As Campbell has said, the thing is that this (and -mmemory-model=tso) currently
does not work when targeting v7 because all the barrier emitters are gated with
TARGET_V8 || TARGET_V9.

(By the way, `-mcpu=v7 -mmemory-model=tso` is broken on Linux too, for the same
reason)

Attached is a patch to relax the barrier requirements such that it is possible
to emit the ldstub barriers even when targeting v7. This does not change any
defaults, but, crucially, it does allow -mmemory-model=tso to be used with v7
target.
There is probably some better way to do it - I am unfamiliar with GCC internals
- but so far it has been working fine to me.

What do you think? Would it be okay if it is only changed in this way?

[Bug target/110592] [SPARC] GCC should default to TSO memory model when compiling for sparc32

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110592

--- Comment #9 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> I don't understand, how would that help?  As I understand it, whenever
> `-mcpu=v7', the memory model is just ignored -- even if we set it to TSO --
> because all rules that depend on it are gated on TARGET_V8 || TARGET_V9 or
> simila

Well, the subject of the PR is "GCC should default to TSO memory model when
compiling for sparc32" so you'll get exactly that.

> I'm not asking for you to change defaults in Linux or Solaris -- I'm just
> asking to be _able_ to say `-mcpu=v7 -mmemory-model=tso' and get v7-only
> instruction streams with the LDSTUBs needed for TSO.  Right now, with
> `-mcpu=v7', passing `-mmemory-model=tso' has no effect.

I'm suggesting changing the default *like* on Linux or Solaris, not to change
anything on Linux or Solaris.

So you want to mix memory models and synchronization instructions with
-mcpu=v7, although they were introduced in the V8 architecture?

[Bug target/110592] [SPARC] GCC should default to TSO memory model when compiling for sparc32

[campbell+gcc-bugzilla at mumble dot net]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110592

--- Comment #10 from Taylor R Campbell <campbell+gcc-bugzilla at mumble dot net> ---
(In reply to Eric Botcazou from comment #9)
> > I don't understand, how would that help?  As I understand it, whenever
> > `-mcpu=v7', the memory model is just ignored -- even if we set it to TSO --
> > because all rules that depend on it are gated on TARGET_V8 || TARGET_V9 or
> > simila
> 
> Well, the subject of the PR is "GCC should default to TSO memory model when
> compiling for sparc32" so you'll get exactly that.

But defaulting to TSO doesn't seem to help with generating LDSTUB in
sparcv7-only instruction streams, unless I misunderstand how this is different
from trying to combine `-mcpu' and `-mmemory-model'?

> So you want to mix memory models and synchronization instructions with
> -mcpu=v7, although they were introduced in the V8 architecture?

Correct.  The idea is to have a way to generate code that works both on sparcv7
-- by avoiding v8-only instructions like SMUL/UMUL, as `-mcpu=v7' does -- and
on sparcv8 -- by generating LDSTUB instructions where store-before-load
ordering is needed, as `-mcpu=v8 -mmemory-model=tso' does.  I tried to spell
this request as `-mcpu=v7 -mmemory-model=tso' but that doesn't generate the
LDSTUB instructions needed for store-before-load ordering.

(Note that LDSTUB is available in v7 -- what's new in v8 is the relaxation of
store-before-load order of TSO, in contrast to SC.  So these requirements
aren't contradictory.)

Is that how Linux and Solaris work by default?  I wasn't able to elicit that
behaviour by combining explicit `-mcpu' and `-mmemory-model' options, so I
assumed that it wouldn't be possible for it to be the default -- and I don't
see how it could work given how the code generation rules for memory barriers
are gated on TARGET_V8 || TARGET_V9 or similar.