This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ro on Jan 24 2022, 2:10 AM.

Details

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.

Diff Detail

Event Timeline

ro created this revision.Jan 24 2022, 2:10 AM
ro requested review of this revision.Jan 24 2022, 2:10 AM

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 added inline comments.Jan 24 2022, 2:16 AM
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 added a subscriber: joerg.Jan 24 2022, 2:40 AM
joerg added inline comments.
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 added inline comments.Jan 24 2022, 2:43 AM
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 added inline comments.Jan 24 2022, 2:45 AM
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 added inline comments.Jan 24 2022, 2:49 AM
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 added inline comments.Jan 24 2022, 2:56 AM
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 added inline comments.Jan 24 2022, 3:05 AM
clang/lib/Driver/ToolChains/Solaris.cpp
138–139

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

ro updated this revision to Diff 403604.Jan 27 2022, 4:56 AM
  • Clarify comment.
  • Use __ATOMIC_SEQ_CST.
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2022, 4:56 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
ro updated this revision to Diff 403607.Jan 27 2022, 4:59 AM
ro marked an inline comment as done.

Omit sanitizer_atomic_clang.h part, belongs to D118021.

glaubitz added inline comments.Jan 31 2022, 8:40 AM
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 added inline comments.Jan 31 2022, 8:48 AM
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 added inline comments.Jan 31 2022, 12:09 PM
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 added a comment.Feb 1 2022, 8:06 PM

Ping? It's been a week.

ro added a comment.Feb 9 2022, 12:25 AM

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.

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 added a comment.Feb 9 2022, 4:24 AM

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 updated this revision to Diff 407120.Feb 9 2022, 4:26 AM

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 accepted this revision.Feb 10 2022, 12:08 AM
MaskRay added inline comments.
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

This revision is now accepted and ready to land.Feb 10 2022, 12:08 AM
ro marked an inline comment as done.Feb 10 2022, 2:53 AM
ro added inline comments.
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 updated this revision to Diff 407455.Feb 10 2022, 3:34 AM
ro marked an inline comment as done.

Simplify test.

ro added inline comments.Feb 10 2022, 3:36 AM
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.

This revision was landed with ongoing or failed builds.Feb 10 2022, 3:40 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2022, 10:25 PM
Herald added a subscriber: StephenFan. · View Herald Transcript