Split LLVM intrinsic abi handling from the rest of the abi handling

[bjorn3]

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.

[rustbot]

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

[rustbot]

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

[bjorn3]

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.

[sayantn]

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

[bjorn3]

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

[sayantn]

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.

[sayantn]

@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

[bjorn3]

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.

[bjorn3]

Something like this @sayantn?

@antoyo @GuillaumeGomez would you mind reviewing the cg_gcc changes?

[sayantn]

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]

@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?

[bjorn3]

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]

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

[antoyo]

./x.py test --test-codegen-backend=gcc should do it.

Isn't that only running UI tests?

[GuillaumeGomez]

No, any testsuite with this argument will use the GCC backend.

[antoyo]

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.

[bjorn3]

Maybe you could build rustc with cg_gcc as default backend and then manually run the stdarch test suite using it?

[sayantn]

Ah sorry, the current design looks nice. I will test it on stdarch locally when I get some time

[antoyo]

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

[bjorn3]

Does it work with the last commit reverted?

[bjorn3]

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.

[antoyo]

Nice. Were you able to also run the stdarch tests or do you want me to do it?

[bjorn3]

I haven't tried. I ran out of memory when building stage 2 rustc_middle several times forcing me to reboot.

[antoyo]

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.

[antoyo]

The stdarch tests pass with this branch.

View changes since this review

[WaffleLapkin]

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

[bjorn3]

The last commit in this PR makes #149141 no longer necessary. I will close #149141 if the approach in that commit is considered acceptable.

[WaffleLapkin]

@bjorn3 my understanding was that the last commit here is a temporary hack that will be later replaced with something proper, is that not the case?

[bjorn3]

Maybe a proper way can be found in the future, but for now I think this hack will stay. Wasm has exactly two llvm intrinsics which need it (llvm.wasm.throw and llvm.wasm.rethrow the latter of which we don't use) and I don't think any other architecture has unwinding llvm intrinsics.

[WaffleLapkin]

@bors r+

[bors]

📌 Commit 4eb6e1c has been approved by WaffleLapkin

It is now in the queue for this repository.

[JonathanBrouwer]

@bors r-
#150404 (comment)

[rustbot]

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.

[bjorn3]

I can't reproduce this locally after a rebase.

[rust-log-analyzer]

The job pr-check-2 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

    |
    = help: items from traits can only be used if the trait is in scope
help: one of the expressions' fields has a method of the same name
    |
627 |                 let fn_abi = self.cx.tcx.fn_abi_of_instance(instance, ty::List::empty());
    |                                   +++++++
help: one of the expressions' fields has a method of the same name
    |
627 |                 let fn_abi = self.tcx.fn_abi_of_instance(instance, ty::List::empty());
    |                                   ++++
help: trait `FnAbiOf` which provides `fn_abi_of_instance` is implemented but not in scope; perhaps you want to import it
    |
  1 + use crate::rustc_middle::ty::layout::FnAbiOf;
    |
help: there is a method `fn_abi_of_fn_ptr` with a similar name
    |

[bjorn3]

I think it is because of the master cargo feature being different between CI and locally.