-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Correct alignment of atomic load and stores #19482
Conversation
b5b5b74
to
8c175e1
Compare
# Note for PPC: | ||
# The OpenPower ABI for Linux Supplement -- 64-Bit ELF V2 ABI Specification | ||
# defines that the alignments of types is sizeof(T). | ||
if Sys.ARCH === :x86_64 || Sys.ARCH === :i686 || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alignment here should not be what the ABI require, but what we can guarantee. See jl_gc_alignment
for the correct definition.
@yuyichao I decided to reuse the information from |
@@ -41,6 +41,13 @@ | |||
extern "C" { | |||
#endif | |||
|
|||
// Make gc alignment available for threading | |||
// see threads.jl alignment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't define functions before headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. sorry about that
This makes sure that atomic load and stores have the correct alignment as guaranteed by the gc.
a9eddc8
to
f8e3e12
Compare
Squashed the pull-request. I will wait on travis and merge this tomorrow. |
While updating LLVM from 3.7 to 3.9 on windows, I noticed that for 128bit atomics a libcall was issued,
instead of resolving to the hardware instruction (that we forcefully enable with the subtarget feature on X86
cx16
).Disabling the
libatomic
support injitlayers.cpp
revealed the same regression on Linux.After discussion with @yuyichao (thank you again) we realized that the X86 ABI of atomic types requires an alignment of
sizeof(T)
instead of the current
WORD_SIZE/8
(see the libatomic ABI draft https://gcc.gnu.org/ml/gcc/2016-11/txt6ZlA_JS27i.txt).Load and stores should follow the alignment guarantees of the GC.
@tkelman this fixes the issue in #19472 properly and after merging this we will only need the SSP patch, from that PR.