compiler_rt: aarch64 outline atomics #11828
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
devins2518
force-pushed
the
arm-atomics
branch
12 times, most recently
from
ae4d00f to
4a4cd4d
Compare
|
@PiotrSikora could you try this to see if it fixes #10777 ? |
devins2518
force-pushed
the
arm-atomics
branch
4 times, most recently
from
31fc53d to
4fee891
Compare
devins2518
force-pushed
the
arm-atomics
branch
2 times, most recently
from
0a660f1 to
d111ab7
Compare
andrewrk
force-pushed
the
arm-atomics
branch
2 times, most recently
from
2300bba to
d6823f9
Compare
* Rely on libSystem when targeting macOS. * Make tools/gen_outline_atomics.zig more idiomatic. * Remove the CPU detection / auxval checking from compiler_rt. This functionality belongs in a different component. Zig's compiler_rt must not rely on constructors. Instead it will export a symbol for setting the value, and start code can detect and activate it. * Remove the separate logic for inline assembly when the target does or does not have lse support. `.inst` works in both cases.
After this, the machine code generated by zig matches identically to gcc's after the differences in loading the have_lse flag.
andrewrk
force-pushed
the
arm-atomics
branch
from
d6823f9 to
545c311
Compare
|
@devins2518 thank you for implementing this. It took me a long time to get around to merging it but it finally landed in 271cc52. I made some edits to what you have here:
|
|
Thanks so much! It's so awesome to have my first meaningful contribution to zig merged 🎉! I've been planning to rewrite this a bit since #12756 was merged which would remove the need for the generation script and simplify |
|
Follow-up: #13622 @devins2518 let's chat with @MasterQ32 about inline assembly - right now zig is at a fork in the road about whether to proceed with string concatenation or a more sophisticated inline assembly syntax built into the language. |
andrewrk
added a commit
that referenced
this pull request
Jan 9, 2023
compiler_rt: aarch64 outline atomics
messense
added a commit
to rust-cross/cargo-zigbuild
that referenced
this pull request
Jan 18, 2023
zig aarch64 outline atomics was resolved in ziglang/zig#11828 [ci skip]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ported from llvm's port of libgcc's outline atomics.
Includes #11667 though, I can remove if this pr is merged before hazes.RemovedFixes #10086 and
zig c++as noted here.Issues
The code is admittedly pretty ugly. There's a lot of inline assembly created through a mixture of multiline strings and regular string concatenation which is not ideal. Not sure if the level of comptime variable parsing is possible through just using inputs for inline assembly.__aarch64_ldadd4_acq_relis tested as it was the test case, but I'm not too sure about all of the other functions.__aarch64_have_lse_atomicsis constructed through a function in the.ctorssection which is definitely hidden control flow. Would be interesting to see any more ziggy solutions to fixing this.__init_aarch64_have_lse_atomicsneeds to be linked into either__DATA,__mod_init_funcor.init_arraydepending on platform, but@exportsection handling isn't implemented. Right now, it is just called at the beginning of each outlined function.initHaveLseAtomicsalso callsstd.os.linux.getauxvaleven on macOS, which doesn't seem right. This seems beyond the scope of this PR though.