Skip to content
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

Closed
wants to merge 5 commits into from

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Jul 20, 2023

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

@rustbot
Copy link
Collaborator

rustbot commented Jul 20, 2023

r? @fee1-dead

(rustbot has picked a reviewer for you, use r? to override)

@rustbot
Copy link
Collaborator

rustbot commented Jul 20, 2023

⚠️ Warning ⚠️

  • These commits modify submodules.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 20, 2023
@rust-log-analyzer

This comment has been minimized.

@tgross35
Copy link
Contributor Author

tgross35 commented Jul 20, 2023

Reports from running abi-cafe:

report-old.txt
report-new.txt

It says that ui128::c::rustc_calls_clang went from failing to passing, but ui128::c::rustc_calls_gcc tests still fail (e.g. i128_val_in_0_perturbed_big). Trying to chase down why that would be.

Invoked by setting /usr/local/bin symlinks to old or new versions then running cargo run -- --pairs gcc_calls_clang clang_calls_gcc rustc_calls_clang clang_calls_rustc rustc_calls_gcc gcc_calls_rustc 2>&1 | tee report-old.txt

@tgross35
Copy link
Contributor Author

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.

reports-and-src-2023-07-20.tar.gz

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 27, 2023

☔ The latest upstream changes (presumably #114105) made this pull request unmergeable. Please resolve the merge conflicts.

@fee1-dead
Copy link
Member

r? @petrochenkov

@rustbot rustbot assigned petrochenkov and unassigned fee1-dead Jul 30, 2023
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2023
@tgross35
Copy link
Contributor Author

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

@tgross35
Copy link
Contributor Author

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).

@tgross35
Copy link
Contributor Author

tgross35 commented Aug 17, 2023

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 ../rust-build-llvm, and llvm itself (with the patches) is at ../llvm-project

cargo build --release
podman run --rm -it \
-v $(pwd):/abi-cafe \
-v $(realpath ../llvm-project/build):/llvm \
-v $(realpath ../rust-build-llvm/build):/rustc \
gcc bash

ln -s /rustc/x86_64-unknown-linux-gnu/stage2/bin/rustc /usr/local/bin/
ln -s /llvm/bin/clang /usr/local/bin/
cd abi-cafe
target/release/abi-cafe --pairs gcc_calls_clang clang_calls_gcc rustc_calls_clang clang_calls_rustc rustc_calls_gcc gcc_calls_rustc 2>&1 | tee report-d86310-d15816.txt

@tgross35
Copy link
Contributor Author

Looks like we may be all good here from an ABI standpoint

report-d86310-d15816.txt

@tgross35
Copy link
Contributor Author

Superceded by #116641

@tgross35 tgross35 closed this Oct 11, 2023
@tgross35 tgross35 deleted the llvm-i128-align-change branch April 10, 2024 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants