x86: Correctly pass larger structs/types in registers with -Zregparm

[sulix]

Fixes #145694. I think this should be the last compiler fix needed to unblock 32-bit x86 support for Rust-for-linux (Rust-for-Linux/linux#78)

Tracking issue for -Zregparm: #131749

The -Zregparm option for 32-bit x86 modifies the calling convention to pass a number of arguments in registers instead of on the stack. (This is primarily used by the Linux kernel.)

Currently, rustc will only pass primitive integer types in registers, which seems to match the gcc documentation[1], which says that arguments "of integral type" are passed as registers. This also matches the fastcall and vectorcall conventions on windows.

However, it seems that any type — most particularly structs — should be passed as a register if possible. This matches what clang and gcc do, and so avoids an ABI mismatch when linking with C code (which, after all, is the whole point of supporting -Zregparm).

Note that I haven't tested this particularly thoroughly outside the Linux kernel usecase, and in particular haven't tried to implement any 'assembly-llvm' tests, nor test for regressions against fastcall/vectorcall.

[rustbot]

r? @jdonszelmann

rustbot has assigned @jdonszelmann.
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]

The PassMode::Ignore | PassMode::Indirect { attrs: _, meta_attrs: None, on_stack: _ } => { continue; } above prevents hitting this code for most structs.

[jdonszelmann]

If this fixes that issue, I'd love to see a test for that issue which now passes. At least some amount of testing seems wise. I'm aware this is an unstable option I know little about otherwise, but since there is a specific test failing, I think adding it as a test is the least we can do before merging this fix so we won't forget about this failure in the future.

[jdonszelmann]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[tgross35]

Does this make us match Clang's LLVM IR?

There are two tests that should probably be updated, tests/codegen-llvm/regparm-inreg.rs to make sure we're matching what Clang does, and tests/assembly-llvm/regparm-module-flag.rs to verify we're passing as expected (that one could just be renamed to regparm.rs, no need to only be the module flag regression test).

[rustbot]

This PR was rebased onto a different master 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.

[SpriteOvO]

@sulix Hi, ping from triage team. This PR has been inactive for a while. Are there any updates on this? Thanks

[Dylan-DPC]

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

[sulix]

I've had a chance to give this another look:
sulix@regparm-struct

Getting the structs to work required a bit of refactoring of the x86-32 ABI to do the inreg handling inline in compute_abi_info, rather than as a separate second pass. In this initial version, that also means that there's now some extra code duplication between x86.rs and x86_win32.rs.

I've also added an extra assembly-llvm test for the struct handling (though I fear it could be fragile: the generated code is not exactly optimal), and some codegen-llvm tests. Manually, I've successfully built and booted an i386 Rust-for-Linux kernel with this, and (unlike without this patch) all KUnit tests pass.

Nevertheless, this could definitely do with some more serious review and testing, as it's both my first Rust patch and changes more of the x86 callconv code than I'd really have liked.

Doesn't look like I have permission to reopen this PR myself: feel free to do so, otherwise I can start a new one if that's more convenient.

[tgross35]

Doesn't look like I have permission to reopen this PR myself: feel free to do so, otherwise I can start a new one if that's more convenient.

For what it's worth, the reopen button is greyed out for me as well. So I think it needs a new PR :)

[tgross35]

Also please ping me on the other PR, I can probably give some guidance on the tests.

[sulix]

Thanks — I've opened #161027.