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@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@gcc202:~$ gcc -m32 -mv8plus atomic.c -o atomic
> > > glaubitz@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@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@gcc202:~$ gcc -m32 -mv8plus atomic.c -o atomic
> > > > glaubitz@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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
