Skip to content

Use a ArrayVec in CastTarget#157130

Open
alexcrichton wants to merge 1 commit into
rust-lang:mainfrom
alexcrichton:refactor-cast-target-internals
Open

Use a ArrayVec in CastTarget#157130
alexcrichton wants to merge 1 commit into
rust-lang:mainfrom
alexcrichton:refactor-cast-target-internals

Conversation

@alexcrichton
Copy link
Copy Markdown
Member

@alexcrichton alexcrichton commented May 29, 2026

View all comments

This commit switches a fixed-size list of [Option<Reg>; 8] to instead
holding ArrayVec<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. For now though this mostly just switches to
array/slice-like idioms of accessors rather than dealing with
Option<Reg> as the unit.

@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

This comment has been minimized.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

They hypothetical wasm use case I have will need the more-than-8-is-possible behavior of this, but that's still in the works as well. Would you prefer to have the interim period use ArrayVec and only switch to SmallVec when necessary?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So... the current code has a hard limit of 8. It's unclear why that was chosen and if it's meaningful. ArrayVec<8> would preserve that limit while making the code nicer.

Allowing arbitrary lengths seems reasonable, but SmallVec<8> says to me "this vec frequently has up to 8 elements and not heap allocating them is important for performance". Which probably isn't true in this case? I bet the most common case is a single element, in which case SmallVec<1> would probably make more sense. But if this code isn't hot then Vec would also be fine and simplest.

So I see two sensible options:

  • Use ArrayVec<8> and add a comment about the 8 probably being arbitrary.
  • Use Vec.

I don't have a strong preference. I guess it comes down to how confident you are that you'll end up needing the arbitrary length.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ok I don't want to rock the boat too much yet, so I've switched to using ArrayVec everywhere. I'm not sure if this is a performance hot spot or not to know whether Vec is worth it over SmallVec

Comment thread compiler/rustc_target/src/callconv/mod.rs Outdated
@alexcrichton alexcrichton force-pushed the refactor-cast-target-internals branch from 7bba23d to 6fee567 Compare May 31, 2026 16:17
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 31, 2026

The GCC codegen subtree was changed

cc @antoyo, @GuillaumeGomez

This commit switches a fixed-size list of `[Option<Reg>; 8]` to instead
holding `ArrayVec<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. For now though this mostly just switches to
array/slice-like idioms of accessors rather than dealing with
`Option<Reg>` as the unit.

[link]: https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Using.20.60ArgAbi.3A.3Amake_direct_deprecated.60/with/598607139
@alexcrichton alexcrichton force-pushed the refactor-cast-target-internals branch from 6fee567 to c668b83 Compare June 1, 2026 14:34
@alexcrichton alexcrichton changed the title Use a SmallVec in CastTarget Use a ArrayVec in CastTarget Jun 1, 2026
@nnethercote
Copy link
Copy Markdown
Contributor

Thanks, the ArrayVec code looks good. I'll be surprised if this is hot, but let's play it safe.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 1, 2026
@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Jun 1, 2026
…r=<try>

Use a `ArrayVec` in `CastTarget`
@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented Jun 1, 2026

☀️ Try build successful (CI)
Build commit: 32cd855 (32cd855918a7ef6654ed0d726a6c1ee9c86eec14, parent: c0bb140a37c81cf59a0b40c21c9413059644e294)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (32cd855): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (secondary 0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
6.2% [6.2%, 6.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-6.1% [-6.1%, -6.1%] 1
All ❌✅ (primary) - - 0

Cycles

Results (secondary 1.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.8% [1.8%, 1.8%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 511.375s -> 509.277s (-0.41%)
Artifact size: 400.63 MiB -> 400.66 MiB (0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 1, 2026
@nnethercote
Copy link
Copy Markdown
Contributor

@bors r+ rollup

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented Jun 1, 2026

📌 Commit c668b83 has been approved by nnethercote

It is now in the queue for this repository.

@rust-bors rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 1, 2026
jhpratt added a commit to jhpratt/rust that referenced this pull request Jun 2, 2026
…internals, r=nnethercote

Use a `ArrayVec` in `CastTarget`

This commit switches a fixed-size list of `[Option<Reg>; 8]` to instead
holding `ArrayVec<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. For now though this mostly just switches to
array/slice-like idioms of accessors rather than dealing with
`Option<Reg>` as the unit.

[link]: https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Using.20.60ArgAbi.3A.3Amake_direct_deprecated.60/with/598607139
rust-bors Bot pushed a commit that referenced this pull request Jun 2, 2026
Rollup of 8 pull requests

Successful merges:

 - #157240 (Enable Enzyme for aarch64-apple-darwin)
 - #157276 (miri subtree update)
 - #154742 (Add APIs for case folding to the standard library)
 - #157130 (Use a `ArrayVec` in `CastTarget`)
 - #157195 (Move feature gating to the new attr parsing infrastructure)
 - #157256 (tests: adapt for LLVM codegen change)
 - #157265 (Update books)
 - #157277 (triagebot.toml: add LawnGnome to libs reviewers)
jhpratt added a commit to jhpratt/rust that referenced this pull request Jun 2, 2026
…internals, r=nnethercote

Use a `ArrayVec` in `CastTarget`

This commit switches a fixed-size list of `[Option<Reg>; 8]` to instead
holding `ArrayVec<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. For now though this mostly just switches to
array/slice-like idioms of accessors rather than dealing with
`Option<Reg>` as the unit.

[link]: https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Using.20.60ArgAbi.3A.3Amake_direct_deprecated.60/with/598607139
rust-bors Bot pushed a commit that referenced this pull request Jun 2, 2026
Rollup of 8 pull requests

Successful merges:

 - #157240 (Enable Enzyme for aarch64-apple-darwin)
 - #157276 (miri subtree update)
 - #157130 (Use a `ArrayVec` in `CastTarget`)
 - #157131 (Add regression test for issue #144888)
 - #157195 (Move feature gating to the new attr parsing infrastructure)
 - #157256 (tests: adapt for LLVM codegen change)
 - #157265 (Update books)
 - #157277 (triagebot.toml: add LawnGnome to libs reviewers)
rust-bors Bot pushed a commit that referenced this pull request Jun 2, 2026
Rollup of 8 pull requests

Successful merges:

 - #157240 (Enable Enzyme for aarch64-apple-darwin)
 - #157276 (miri subtree update)
 - #157130 (Use a `ArrayVec` in `CastTarget`)
 - #157131 (Add regression test for issue #144888)
 - #157195 (Move feature gating to the new attr parsing infrastructure)
 - #157256 (tests: adapt for LLVM codegen change)
 - #157265 (Update books)
 - #157277 (triagebot.toml: add LawnGnome to libs reviewers)
rust-bors Bot pushed a commit that referenced this pull request Jun 2, 2026
Rollup of 8 pull requests

Successful merges:

 - #157240 (Enable Enzyme for aarch64-apple-darwin)
 - #157276 (miri subtree update)
 - #157130 (Use a `ArrayVec` in `CastTarget`)
 - #157131 (Add regression test for issue #144888)
 - #157195 (Move feature gating to the new attr parsing infrastructure)
 - #157256 (tests: adapt for LLVM codegen change)
 - #157265 (Update books)
 - #157277 (triagebot.toml: add LawnGnome to libs reviewers)
rust-bors Bot pushed a commit that referenced this pull request Jun 2, 2026
Rollup of 8 pull requests

Successful merges:

 - #157240 (Enable Enzyme for aarch64-apple-darwin)
 - #157276 (miri subtree update)
 - #157130 (Use a `ArrayVec` in `CastTarget`)
 - #157131 (Add regression test for issue #144888)
 - #157195 (Move feature gating to the new attr parsing infrastructure)
 - #157256 (tests: adapt for LLVM codegen change)
 - #157265 (Update books)
 - #157277 (triagebot.toml: add LawnGnome to libs reviewers)
rust-bors Bot pushed a commit that referenced this pull request Jun 2, 2026
Rollup of 8 pull requests

Successful merges:

 - #157240 (Enable Enzyme for aarch64-apple-darwin)
 - #157276 (miri subtree update)
 - #157130 (Use a `ArrayVec` in `CastTarget`)
 - #157131 (Add regression test for issue #144888)
 - #157195 (Move feature gating to the new attr parsing infrastructure)
 - #157256 (tests: adapt for LLVM codegen change)
 - #157265 (Update books)
 - #157277 (triagebot.toml: add LawnGnome to libs reviewers)
rust-bors Bot pushed a commit that referenced this pull request Jun 2, 2026
Rollup of 8 pull requests



Successful merges:

 - #157240 (Enable Enzyme for aarch64-apple-darwin)
 - #157276 (miri subtree update)
 - #157130 (Use a `ArrayVec` in `CastTarget`)
 - #157131 (Add regression test for issue #144888)
 - #157195 (Move feature gating to the new attr parsing infrastructure)
 - #157256 (tests: adapt for LLVM codegen change)
 - #157265 (Update books)
 - #157277 (triagebot.toml: add LawnGnome to libs reviewers)
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Jun 2, 2026
…internals, r=nnethercote

Use a `ArrayVec` in `CastTarget`

This commit switches a fixed-size list of `[Option<Reg>; 8]` to instead
holding `ArrayVec<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. For now though this mostly just switches to
array/slice-like idioms of accessors rather than dealing with
`Option<Reg>` as the unit.

[link]: https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Using.20.60ArgAbi.3A.3Amake_direct_deprecated.60/with/598607139
rust-bors Bot pushed a commit that referenced this pull request Jun 2, 2026
…uwer

Rollup of 14 pull requests

Successful merges:

 - #156467 (mark the 'import linkage' statics as unnamed_addr)
 - #156923 (Couple of diagnostics improvements for EII)
 - #157240 (Enable Enzyme for aarch64-apple-darwin)
 - #157244 (Privacy: tweak macros + more tests)
 - #157276 (miri subtree update)
 - #157130 (Use a `ArrayVec` in `CastTarget`)
 - #157131 (Add regression test for issue #144888)
 - #157195 (Move feature gating to the new attr parsing infrastructure)
 - #157233 (rustdoc: Fix trait impl ordering)
 - #157256 (tests: adapt for LLVM codegen change)
 - #157265 (Update books)
 - #157277 (triagebot.toml: add LawnGnome to libs reviewers)
 - #157291 (Clean up attribute target checking diagnostics)
 - #157301 (Remove unused import from a test)
rust-bors Bot pushed a commit that referenced this pull request Jun 2, 2026
…uwer

Rollup of 14 pull requests

Successful merges:

 - #156467 (mark the 'import linkage' statics as unnamed_addr)
 - #156923 (Couple of diagnostics improvements for EII)
 - #157240 (Enable Enzyme for aarch64-apple-darwin)
 - #157244 (Privacy: tweak macros + more tests)
 - #157276 (miri subtree update)
 - #157130 (Use a `ArrayVec` in `CastTarget`)
 - #157131 (Add regression test for issue #144888)
 - #157195 (Move feature gating to the new attr parsing infrastructure)
 - #157233 (rustdoc: Fix trait impl ordering)
 - #157256 (tests: adapt for LLVM codegen change)
 - #157265 (Update books)
 - #157277 (triagebot.toml: add LawnGnome to libs reviewers)
 - #157291 (Clean up attribute target checking diagnostics)
 - #157301 (Remove unused import from a test)
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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

5 participants