Page MenuHomePhabricator

[LLVM] Add libatomic load/store functions to TargetLibraryInfo
ClosedPublic

Authored by guiand on Jul 7 2020, 5:08 PM.

Details

Summary

This allows treating these functions like libcalls.

This patch is a prerequisite to instrumenting them in MSAN:

https://reviews.llvm.org/D83337

Diff Detail

Event Timeline

guiand created this revision.Jul 7 2020, 5:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2020, 5:08 PM
rovka added a subscriber: rovka.Jul 8 2020, 1:47 AM

Don't you also have to set as Available/Unavailable when initializing the TLI?

llvm/lib/Analysis/TargetLibraryInfo.cpp
1232

Nit: The comment should go above the case statement.

Don't you also have to set as Available/Unavailable when initializing the TLI?

Yes, thanks for the catch! It looks like it's set available by default, there may be platforms that don't have these libatomic functions available.

From a look at RuntimeLibCalls, where these functions are also used (for emitting), it looks like the functions are only explicitly made unavailable on webassembly. (Which is pretty surprising! Could I be missing something here?)

I had gotten the impression that these calls aren't supported on WebAssembly by lines in WebAssemblyRuntimeLibcallSignatures.cpp like:

Table[RTLIB::ATOMIC_LOAD] = unsupported;

But it turns out clang has no problem generating calls to __atomic_load anyway: https://gcc.godbolt.org/z/kMDLJb

And it looks like LLVM looks to provide general support for these functions: https://llvm.org/docs/Atomics.html#libcalls-atomic

rovka added a comment.Jul 10 2020, 4:18 AM

Hi again,

I'm sorry, but I don't know much about WebAssembly.

I would suggest taking the opposite approach: mark as unavailable in general, and as available only on platforms where you know for sure they exist - based on the docs you linked, that's probably anything that has C++11 support.

Maybe someone else with more experience in this area can chime in.

jfb added a subscriber: dschuff.Jul 10 2020, 9:58 AM

@dschuff can help answer the WebAssembly question.

guiand updated this revision to Diff 278000.Jul 14 2020, 3:24 PM

I've added a short list of architectures known to use atomic operations and observed to generate calls to __atomic_load/store

rovka added a comment.Jul 16 2020, 1:37 AM

Thanks for the update. This looks fine to me as is, but I'll defer the final LGTM to someone that knows this area a bit better.

llvm/lib/Analysis/TargetLibraryInfo.cpp
175

Nit: this doesn't need to move.

guiand marked an inline comment as done.Jul 16 2020, 9:25 AM
guiand added inline comments.
llvm/lib/Analysis/TargetLibraryInfo.cpp
175

I moved it just up one scope so I can use it in the if statement below.

efriedma added inline comments.Jul 16 2020, 1:07 PM
llvm/lib/Analysis/TargetLibraryInfo.cpp
555

Checking for specific targets seems like a trap that's going to "randomly" break more obscure targets.

I think I'd be happy to say these are "always" available, in the sense that we should assume a function named __atomic_load is always the libatomic one. (In practice, whether they're actually available at link-time might vary depending on how the user links their code, but we shouldn't be introducing new calls to these anyway.)

guiand updated this revision to Diff 278592.Jul 16 2020, 1:18 PM

@efriedma that makes sense. I've reverted to the previous patch where these functions are always available.

guiand marked an inline comment as done.Jul 16 2020, 1:19 PM
This revision is now accepted and ready to land.Jul 16 2020, 1:20 PM
vitalybuka accepted this revision.Jul 17 2020, 12:22 AM
This revision was automatically updated to reflect the committed changes.