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

[ro]

Summary

Even after D86621, clang -m32 on Solaris/sparcv9 doesn't inline atomics with 8-byte
operands, unlike gcc. This leads to many link failures in the testsuite (undefined references
to __atomic_load_8 and __sync_val_compare_and_swap_8. Until a proper codegen fix can be implemented, this patch works around the first of those by linking with -latomic.

Tested on sparcv9-sun-solaris2.11.

[glaubitz]

Ah, I actually ran into this very problem! Thanks for fixing this.

Will test this today on Linux and report back!

Could you maybe have a look at my updated revision here? https://reviews.llvm.org/D98575

I think this particular change should be save on Solaris as well.

[glaubitz]

clang/lib/Driver/ToolChains/Solaris.cpp

138–139

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).

[joerg]

clang/lib/Driver/ToolChains/Solaris.cpp

135

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.

[glaubitz]

clang/lib/Driver/ToolChains/Solaris.cpp

135

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:~$

[ro]

clang/lib/Driver/ToolChains/Solaris.cpp

135

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.

[glaubitz]

clang/lib/Driver/ToolChains/Solaris.cpp

135

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

[ro]

clang/lib/Driver/ToolChains/Solaris.cpp

135

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.

135

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.

138–139

True, that the summary said this patch works around the first of those. The other part is now D118024.

[glaubitz]

clang/lib/Driver/ToolChains/Solaris.cpp

138–139

Ah, sorry. I missed that he was specifically talking about SPARCv8.

[ro]

• Clarify comment.
• Use __ATOMIC_SEQ_CST.

[ro]

Omit sanitizer_atomic_clang.h part, belongs to D118021.

[glaubitz]

clang/lib/Driver/ToolChains/Solaris.cpp

135

Interesting. Would SparcInstr64Bit.td actually get used when targeting 32-bit SPARC?

If yes, it looks like replacing the guards Is64Bit with HasV9 should do the trick, shouldn't it?

[glaubitz]

clang/lib/Driver/ToolChains/Solaris.cpp

135

Ah, the header of SparcInstr64Bit.td actually has the following comment:

// This file contains instruction definitions and patterns needed for 64-bit
// code generation on SPARC v9.
//
// Some SPARC v9 instructions are defined in SparcInstrInfo.td because they can
// also be used in 32-bit code running on a SPARC v9 CPU.

So, I guess move the 64-bit atomics stuff out of SparcInstr64.td and guard it with HasV9 instead of Is64Bit?

[efriedma]

clang/lib/Driver/ToolChains/Solaris.cpp

135

The hard part is adding support for using 64-bit registers on 32-bit targets, in general. You need to declare the register class correctly, and call addRegisterClass() in SparcISelLowering.cpp to declare that 64-bit integer registers exist. Once you mark the 64-bit registers legal, legalization rules for a bunch of basic operations will change, so you'll need to do a bunch of work to get basic 64-bit arithmetic/load/store/etc. working again.

Once you have all that, telling isel to emit 64-bit atomics should be easy.

[ro]

Ping? It's been a week.

[ro]

Ping^2. It would be great if this could be reviewed/commited soon: it fixes hundreds of testsuite failures, so I'd like to get it into LLVM 14.

Thanks.

[MaskRay]

This needs some tests, otherwise there is a risk that others may break your changes when refactoring driver code.

The style I favor most is test/Driver/linux-cross.cpp. freebsd.c is not too bad. linux-ld.c is somewhat messy but can give you some idea what to test.
I'll add a note that zignore isn't tested and that is a problem.

[ro]

In D118021#3307079, @MaskRay wrote:

This needs some tests, otherwise there is a risk that others may break your changes when refactoring driver code.

I thought it would be enough that any breakage there would cause a large number of test failures. But it's certainly better to avoid problems up from rather than reacting afterwards.

The style I favor most is test/Driver/linux-cross.cpp. freebsd.c is not too bad. linux-ld.c is somewhat messy but can give you some idea what to test.
I'll add a note that zignore isn't tested and that is a problem.

It seemed easiest to add to solaris-ld.c.

[ro]

Add tests in solaris-ld.c.

Spot-checked by running the single test with llvm-lit on sparcv9-sun-solaris2.11,
amd64-solaris2.11, and x86_64-pc-linux-gnu.

[MaskRay]

clang/test/Driver/solaris-ld.c

20 ↗

(On Diff #407120)

If they are consecutive.

46 ↗

(On Diff #407120)

Such NOT patterns are usually inadequate and may go stale pretty easily, since technically the patterns can occur in many places.

One idea is to use --implicit-check-not; another is to enumerate all options and use the {{^}} style I picked in linux-cross.cpp, but perhaps your style is good enough if we can remember these library after after -L and before -lgcc_s

[ro]

clang/test/Driver/solaris-ld.c

20 ↗

(On Diff #407120)

The are, patch amended.

46 ↗

(On Diff #407120)

Such NOT patterns are usually inadequate and may go stale pretty easily, since technically the patterns can occur in many places.

I wondered so myself: while this is currently the only instance of -zignore/-zrecord, -lgcc_s requires similar treatment.

One idea is to use --implicit-check-not; another is to enumerate all options and use the {{^}} style I picked in linux-cross.cpp, but perhaps your style is good enough if we can remember these library after after -L and before -lgcc_s

I'll check those. However, it occured to me that the crucial check is that -latomic isn't added at all, -zignore/-zrecord or no, so maybe just check for that and avoid the issue for the moment.

[ro]

Simplify test.

[ro]

clang/test/Driver/solaris-ld.c

46 ↗

(On Diff #407120)

In the end, I've decided to go for the simple *-NOT: "-latomic" form: it's simple and robust, which is all the more important since this patch needs to go into the release/14.x` branch, too.

Going forward, I thing going for the linux-cross.cpp style is best.

clang/lib/Driver/ToolChains/CommonArgs.h

 void addArchSpecificRPath(const ToolChain &TC, const llvm::opt::ArgList &Args,
                           llvm::opt::ArgStringList &CmdArgs);
 /// Returns true, if an OpenMP runtime has been added.
 bool addOpenMPRuntime(llvm::opt::ArgStringList &CmdArgs, const ToolChain &TC,
                       const llvm::opt::ArgList &Args,
                       bool ForceStaticHostRuntime = false,
                       bool IsOffloadingHost = false, bool GompNeedsRT = false);
 
+const char *getAsNeededOption(const ToolChain &TC, bool as_needed);
+
 llvm::opt::Arg *getLastProfileUseArg(const llvm::opt::ArgList &Args);
 llvm::opt::Arg *getLastProfileSampleUseArg(const llvm::opt::ArgList &Args);
 
 bool isObjCAutoRefCount(const llvm::opt::ArgList &Args);
 
 llvm::StringRef getLTOParallelism(const llvm::opt::ArgList &Args,
                                   const Driver &D);
 

clang/lib/Driver/ToolChains/CommonArgs.cpp

static bool addSanitizerDynamicList(const ToolChain &TC, const ArgList &Args,
   SmallString<128> SanRT(TC.getCompilerRT(Args, Sanitizer));
   if (llvm::sys::fs::exists(SanRT + ".syms")) {
     CmdArgs.push_back(Args.MakeArgString("--dynamic-list=" + SanRT + ".syms"));
     return true;
   }
   return false;
 }
 
-static const char *getAsNeededOption(const ToolChain &TC, bool as_needed) {
+const char *tools::getAsNeededOption(const ToolChain &TC, bool as_needed) {
   assert(!TC.getTriple().isOSAIX() &&
          "AIX linker does not support any form of --as-needed option yet.");
 
   // While the Solaris 11.2 ld added --as-needed/--no-as-needed as aliases
   // for the native forms -z ignore/-z record, they are missing in Illumos,
   // so always use the native form.
   if (TC.getTriple().isOSSolaris())
     return as_needed ? "-zignore" : "-zrecord";

clang/lib/Driver/ToolChains/Solaris.cpp

if (getToolChain().ShouldLinkCXXStdlib(Args))
       getToolChain().AddCXXStdlibLibArgs(Args, CmdArgs);
     if (Args.hasArg(options::OPT_fstack_protector) ||
         Args.hasArg(options::OPT_fstack_protector_strong) ||
         Args.hasArg(options::OPT_fstack_protector_all)) {
       // Explicitly link ssp libraries, not folded into Solaris libc.
       CmdArgs.push_back("-lssp_nonshared");
       CmdArgs.push_back("-lssp");
     }
+    // LLVM support for atomics on 32-bit SPARC V8+ is incomplete, so
+    // forcibly link with libatomic as a workaround.
+    if (getToolChain().getTriple().getArch() == llvm::Triple::sparc) {
+      CmdArgs.push_back(getAsNeededOption(getToolChain(), true));
+      CmdArgs.push_back("-latomic");
+      CmdArgs.push_back(getAsNeededOption(getToolChain(), false));
+    }
     CmdArgs.push_back("-lgcc_s");
     CmdArgs.push_back("-lc");
     if (!Args.hasArg(options::OPT_shared)) {
       CmdArgs.push_back("-lgcc");
       CmdArgs.push_back("-lm");
     }
     if (NeedsSanitizerDeps)
       linkSanitizerRuntimeDeps(getToolChain(), CmdArgs);