-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Use LLVM patch to correct alignments for i128
and u128
#113880
Conversation
r? @fee1-dead (rustbot has picked a reviewer for you, use r? to override) |
|
This comment has been minimized.
This comment has been minimized.
Reports from running abi-cafe: It says that Invoked by setting |
I haven't yet parsed the data, but here is the json output and source c/rs files. Unfortunately GH doesn't allow uploading .json or .c or .rs files (can't think of a good reason) so here's a zip. |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #114105) made this pull request unmergeable. Please resolve the merge conflicts. |
Fwiw this likely won't be ready for a real merge too soon since there are further changes on the llvm side (still does the wrong thing with function calls). But I wouldn't mind a "this does/doesn't look like the right way to do this change" sort of review |
Another note is this will need a perf run before merge since it affects the size of some oft-used structures in the compiler, probably at similar or improved runtime but with a mem hit. This is important enough that I don't think we'd ever block on perf, but it may open some further discussion on whether we might be able to optimize the types (maybe using a wrapper that repr aligns to 8 to keep the current behavior). |
For future reference, this is what I am using to generate the reports. This assumes that https://github.com/Gankra/abi-cafe has been cloned and you have entered that directory, rust with llvm built is located at
|
Looks like we may be all good here from an ABI standpoint |
Superceded by #116641 |
This uses the current patch for https://reviews.llvm.org/D86310, experimenting how to make it work with rustc.
Zulip discussion: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.2354341.20-.20alignment.20of.20i128.20for.20FFI
Note also that I only updated one layout string so far