[PATCH] D118021: [Driver] Use libatomic for 32-bit SPARC atomics support

[Rainer Orth via Phabricator via cfe-commits]

ro added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Solaris.cpp:135
     }
+    // LLVM lacks atomics support on 32-bit SPARC, so forcibly link with
+    // libatomic as a workaround.
----------------
glaubitz wrote:
> ro wrote:
> > glaubitz wrote:
> > > joerg wrote:
> > > > This comment is misleading. It's not so much that LLVM doesn't support them, but that SPARCv8 doesn't have the necessary hardware support. The v8+ support is incomplete, which is a related problem though.
> > > As far as I know, 64-bit atomics are supported if you enable V8+ in GCC - without linking against `libatomic`:
> > > 
> > > ```
> > > glaubitz at gcc202:~$ cat atomic.c
> > > #include <stdint.h>
> > > 
> > > int main()
> > > {
> > >   int64_t x = 0, y = 1;
> > >   y = __sync_val_compare_and_swap(&x, x, y);
> > >   return 0;
> > > }
> > > glaubitz at gcc202:~$ gcc -m32 -mv8plus atomic.c -o atomic
> > > glaubitz at gcc202:~$
> > > ```
> > I know, that's why I referred to my patch to default `clang` on
> > Solaris/sparc to V8+.  I'll update the comment.
> > 
> > I'd tried to actually fix the underlying issue (`clang` not emitting
> > `casx` with `-m32 -mcpu=v9`), but ran into internal errors and
> > areas of LLVM I know nothing about.  I might post a WIP patch
> > for reference since there are several issues there.
> I think @jrtc27 might be able to give advise here having the knowledge on the Tablegen stuff (if my mind serves me right).
> 
> The disassembly shows in any case that `casx` is being emitted as you say:
> 
> ```
> 69c:   c7 f0 50 02     casx  [ %g1 ], %g2, %g3
> ```
Of course it does, thus my reference to `unlike gcc` in the
summary.  What Joerg meant, I believe, is that V8+ support
**in LLVM** is incomplete.


================
Comment at: clang/lib/Driver/ToolChains/Solaris.cpp:135
     }
+    // LLVM lacks atomics support on 32-bit SPARC, so forcibly link with
+    // libatomic as a workaround.
----------------
ro wrote:
> glaubitz wrote:
> > ro wrote:
> > > glaubitz wrote:
> > > > joerg wrote:
> > > > > This comment is misleading. It's not so much that LLVM doesn't support them, but that SPARCv8 doesn't have the necessary hardware support. The v8+ support is incomplete, which is a related problem though.
> > > > As far as I know, 64-bit atomics are supported if you enable V8+ in GCC - without linking against `libatomic`:
> > > > 
> > > > ```
> > > > glaubitz at gcc202:~$ cat atomic.c
> > > > #include <stdint.h>
> > > > 
> > > > int main()
> > > > {
> > > >   int64_t x = 0, y = 1;
> > > >   y = __sync_val_compare_and_swap(&x, x, y);
> > > >   return 0;
> > > > }
> > > > glaubitz at gcc202:~$ gcc -m32 -mv8plus atomic.c -o atomic
> > > > glaubitz at gcc202:~$
> > > > ```
> > > I know, that's why I referred to my patch to default `clang` on
> > > Solaris/sparc to V8+.  I'll update the comment.
> > > 
> > > I'd tried to actually fix the underlying issue (`clang` not emitting
> > > `casx` with `-m32 -mcpu=v9`), but ran into internal errors and
> > > areas of LLVM I know nothing about.  I might post a WIP patch
> > > for reference since there are several issues there.
> > I think @jrtc27 might be able to give advise here having the knowledge on the Tablegen stuff (if my mind serves me right).
> > 
> > The disassembly shows in any case that `casx` is being emitted as you say:
> > 
> > ```
> > 69c:   c7 f0 50 02     casx  [ %g1 ], %g2, %g3
> > ```
> Of course it does, thus my reference to `unlike gcc` in the
> summary.  What Joerg meant, I believe, is that V8+ support
> **in LLVM** is incomplete.
Right, `gcc` does all of this correctly.  One LLVM issue, e.g., is
that it handles `casx` as 64-bit only (cf. `SparcInstr64Bit.td`)
while it should be guarded by `HasV9` instead.


================
Comment at: clang/lib/Driver/ToolChains/Solaris.cpp:138-139
+    if (getToolChain().getTriple().getArch() == llvm::Triple::sparc) {
+      CmdArgs.push_back(getAsNeededOption(getToolChain(), true));
+      CmdArgs.push_back("-latomic");
+      CmdArgs.push_back(getAsNeededOption(getToolChain(), false));
----------------
glaubitz wrote:
> Note, this will only work when `__atomic_compare_exchange()` is being used as ``__sync_val_compare_and_swap_8` is not implemented by `libatomic` in gcc, see: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63368 (Unless this has changed recently).
True, that the summary said `this patch works around the first of those`.  The other part is now D118024.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118021/new/

https://reviews.llvm.org/D118021