Split LLVM intrinsic abi handling from the rest of the abi handling by bjorn3 · Pull Request #148533 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation45 Commits8 Checks11 Files changed
Conversation
This file contains hidden or 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
[ Show hidden characters]({{ revealButtonHref }})
LLVM intrinsics have weird requirements like requiring the fake "unadjusted" abi, not being callable through function pointers and for all codegen backends other than cg_llvm requiring special cases to redirect them to the correct backend specific intrinsic (or directly codegen their implementation inline without any intrinsic call). By splitting the LLVM intrinsic handling it becomes easier for backends to special case them and should in the future allow getting rid of the abi calculation for extern "unadjusted" in favor of computing the correct abi directly in the backend without depending on the exact way cg_ssa lowers types.
Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues.
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
r? @SparrowLii
rustbot has assigned @SparrowLii.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
cc @sayantn This should make #140763 much less intrusive for the non-intrinsic call handling and should allow you to localize all changes to cg_llvm without having to touch cg_ssa and cg_gcc.
this is awesome, thanks ❤️. I tested compiling stdarch with this branch (for x86_64-unknown-linux-gnu, aarch64-unknown-linux-gnu and riscv64gc-unknown-linux-gnu targets), it doesn't generate any errors
Thanks for testing! I hadn't thought about testing this on stdarch. Did you also test compiling the stdarch tests? Otherwise no llvm intrinsic calls are actually getting codegened I think due to all #[inline(always)].
yes, I did cargo build -p core_arch --tests, which will compile all intrinsics in x86 (due to all intrinsics having corresponding tests), I am not so sure about aarch64, there can be some intrinsics that are not tested.
@bjorn3 could you see if it's possible to split the function declaration of LLVM intrinsics too - after inspecting my PR I noticed that it modifies declaration more than calling. Thanks
I figured I would do that after this PR gets merged, but I can do it somewhere in the next couple of days too. In this PR if it hasn't been reviewed by then and otherwise in a separate PR.
Thanks, It's nice, but do we need to cache the intrinsic instances? get_fn should be called only when declaring an intrinsic, and that's mostly one-time for an intrinsic
@antoyo @GuillaumeGomez would you mind reviewing the cg_gcc changes?
This does look good to me, but I would need to run the tests of cg_gcc to confirm.
@GuillaumeGomez: Is there an easy way to run all of cg_gcc's tests in the Rust repo?
It's nice, but do we need to cache the intrinsic instances?
codegen_llvm_intrinsic_call is called once per intrinsic call, not once per intrinsic declaration.
@GuillaumeGomez: Is there an easy way to run all of cg_gcc's tests in the Rust repo?
./x.py test --test-codegen-backend=gcc should do it.
./x.py test --test-codegen-backend=gccshould do it.
Isn't that only running UI tests?
No, any testsuite with this argument will use the GCC backend.
No, any testsuite with this argument will use the GCC backend.
I meant that this won't run the stdarch tests or other tests we have in the CI of the cg_gcc repo.
We need a test that uses LLVM intrinsics and I'm not sure if those we can run in the Rust repo has any.
Maybe you could build rustc with cg_gcc as default backend and then manually run the stdarch test suite using it?
Ah sorry, the current design looks nice. I will test it on stdarch locally when I get some time
Maybe you could build rustc with cg_gcc as default backend and then manually run the stdarch test suite using it?
I tried to build your branch locally and it seems it broke a stage-2 build with cg_gcc.
I get this error:
libgccjit.so: error: : bitcast with types of different sizes
input expression (size: 8):
<integer_cst 0x7f9ff9560348 type <integer_type 0x7f9ff954b498 unsigned char> constant visited 0>
requested type (size: 32):
<integer_type 0x7f9ff954bc78 int public SI
size <integer_cst 0x7f9ff9560468 type <integer_type 0x7f9ff954b690 bitsizetype> constant 32>
unit-size <integer_cst 0x7f9ff9560480 type <integer_type 0x7f9ff954b5e8 sizetype> constant 4>
align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7f9ff954bc78 precision:32 min <integer_cst 0x7f9ff9560420 -2147483648> max <integer_cst 0x7f9ff9560438 2147483647>
pointer_to_this <pointer_type 0x7f9ff95691f8>>
Does it work with the last commit reverted?
Managed to reproduce it locally. I've added 4f4812f, split out be36d2f, removed all cg_gcc changes from 915af4f and finally changed 6d4eb0e to use a function pointer + bx.call() rather than a direct function call. It seems that there is some function signature casting logic in bx.call() that is load bearing here. I didn't manage to figure out exactly what though.
Nice. Were you able to also run the stdarch tests or do you want me to do it?
I haven't tried. I ran out of memory when building stage 2 rustc_middle several times forcing me to reboot.
Oh, let's hope that the next sync that reduced memory usage considerably in some cases would help here as well.
I'll try to run those tests locally.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other two options I can think of are: special case llvm.wasm.throw to take the regular call path or use inline asm in _Unwind_RaiseException to directly lower to .tagtype __cpp_exception i32; local.get 0; throw __cpp_exception, which is what llvm.wasm.throw effectively lowers to.
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
Status: Awaiting review from the assignee but also interested parties.
Status: Blocked on something else such as an RFC or other implementation work.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
Replacing the llvm.wasm.throw usage turns out to be harder than I thought. I've added a commit to avoid using the codegen_llvm_intrinsic_call for llvm.wasm.throw. @sayantn is that ok wrt #140763? The function signature of llvm.wasm.throw is trivial to reproduce in Rust with extern "C-unwind", is not overloaded and if LLVM ever changes the name or signature we can easily change this one call without having to depend on LLVM's auto-upgrade mechanism.
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.
Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.
Status: Blocked on something else such as an RFC or other implementation work.
label
@bjorn3 yeah I have no problem in leaving llvm.wasm.throw out for now. The PR is mostly for intrinsics that can't be reached from Rust, and as you said this intrinsic can be easily specified
Intrinsics only need a fraction of the functionality offered by BuilderMethods::call and in particular don't need the FnAbi to be computed other than (currently) as step towards computing the function value type.
This moves all LLVM intrinsic handling out of the regular call path for cg_gcc and makes it easier to hook into this code for future cg_llvm changes.
As this is the only unwinding intrinsic we use and codegen_llvm_intrinsic_call currently doesn't handle unwinding intrinsics, this uses the conventional call path for llvm.wasm.throw.
What's the status with #149141? Is this PR no longer blocked on it? (I couldn't quite figure out what's the problem with it by reading the comments...)
The last commit in this PR makes #149141 no longer necessary. I will close #149141 if the approach in that commit is considered acceptable.
Labels
Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues.
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.