Skip to content

Use a SmallVec in CastTarget#157130

Open
alexcrichton wants to merge 2 commits into
rust-lang:mainfrom
alexcrichton:refactor-cast-target-internals
Open

Use a SmallVec in CastTarget#157130
alexcrichton wants to merge 2 commits into
rust-lang:mainfrom
alexcrichton:refactor-cast-target-internals

Conversation

@alexcrichton
Copy link
Copy Markdown
Member

This commit switches a fixed-size list of [Option<Reg>; 8] to instead holding SmallVec<[Reg; 8]> in the CastTarget type used when calculating ABIs. This is inspired by discussion on Zulip where I'm hoping to in the near future extend the usage of this to possibly beyond 8 elements for a new WebAssembly ABI taking advantage of multi-value.

This commit switches a fixed-size list of `[Option<Reg>; 8]` to instead
holding `SmallVec<[Reg; 8]>` in the `CastTarget` type used when
calculating ABIs. This is inspired by [discussion on Zulip][link] where
I'm hoping to in the near future extend the usage of this to possibly
beyond 8 elements for a new WebAssembly ABI taking advantage of
multi-value.

[link]: https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Using.20.60ArgAbi.3A.3Amake_direct_deprecated.60/with/598607139
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 29, 2026

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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 May 29, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 29, 2026

r? @nnethercote

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

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 73 candidates
  • Random selection from 17 candidates

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 29, 2026

The Cranelift subtree was changed

cc @bjorn3

@rust-log-analyzer
Copy link
Copy Markdown
Collaborator

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)
    Checking rustc_codegen_gcc v0.1.0 (/checkout/compiler/rustc_codegen_gcc)
error[E0599]: no method named `is_none` found for reference `&rustc_abi::Reg` in the current scope
  --> compiler/rustc_codegen_gcc/src/abi.rs:49:41
   |
49 |         if self.prefix.iter().all(|x| x.is_none()) {
   |                                         ^^^^^^^ method not found in `&rustc_abi::Reg`

error[E0599]: `&rustc_abi::Reg` is not an iterator
  --> compiler/rustc_codegen_gcc/src/abi.rs:65:47
   |
65 |             .flat_map(|option_reg| option_reg.map(|reg| reg.gcc_type(cx)))
   |                                               ^^^ `&rustc_abi::Reg` is not an iterator
   |
  ::: compiler/rustc_abi/src/callconv/reg.rs:21:1
   |
21 | pub struct Reg {
   | -------------- doesn't satisfy `rustc_abi::Reg: std::iter::Iterator`
   |
   = note: the following trait bounds were not satisfied:
           `&rustc_abi::Reg: std::iter::Iterator`
           which is required by `&mut &rustc_abi::Reg: std::iter::Iterator`
           `rustc_abi::Reg: std::iter::Iterator`
           which is required by `&mut rustc_abi::Reg: std::iter::Iterator`

For more information about this error, try `rustc --explain E0599`.
[RUSTC-TIMING] rustc_codegen_gcc test:false 1.176
error: could not compile `rustc_codegen_gcc` (lib) due to 2 previous errors
Bootstrap failed while executing `check`

Copy link
Copy Markdown
Contributor

@nnethercote nnethercote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GCC backend still needs fixing. Probably worth squashing the commits, too; there's no reason for them to be separate.

View changes since this review

/// (and all data in the padding between the registers is dropped).
#[derive(Clone, PartialEq, Eq, Hash, Debug, StableHash)]
pub struct CastTarget {
pub prefix: [Option<Reg>; 8],
Copy link
Copy Markdown
Contributor

@nnethercote nnethercote May 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Option<Reg>; 8], lol, what an awful representation.

If you wanted a nicer representation while still retaining the "at most 8 elements" constraint you could use ArrayVec<[Reg; 8]> instead. That would be very similar to the code you've written, and get the nicer iterations/pushes/etc. Then the change to allow more than 8 elements (later) would just be a conversion to SmallVec.

#[derive(Clone, PartialEq, Eq, Hash, Debug, StableHash)]
pub struct CastTarget {
pub prefix: [Option<Reg>; 8],
pub prefix: SmallVec<[Reg; 8]>,
Copy link
Copy Markdown
Contributor

@nnethercote nnethercote May 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have a comment explaining the 8 here.

View changes since the review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants