Skip to content

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
Collaborator

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