Use a ArrayVec in CastTarget#157130
Conversation
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
|
r? @nnethercote rustbot has assigned @nnethercote. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
|
The Cranelift subtree was changed cc @bjorn3 |
This comment has been minimized.
This comment has been minimized.
| /// (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], |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
7bba23d to
6fee567
Compare
|
The GCC codegen subtree was changed |
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
6fee567 to
c668b83
Compare
SmallVec in CastTargetArrayVec in CastTarget
|
Thanks, the @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…r=<try> Use a `ArrayVec` in `CastTarget`
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (32cd855): comparison URL. Overall result: no relevant changes - no action neededBenchmarking 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 countThis 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.
CyclesResults (secondary 1.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 511.375s -> 509.277s (-0.41%) |
|
@bors r+ rollup |
…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
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)
…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
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)
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)
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)
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)
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)
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)
…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
…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)
…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)
View all comments
This commit switches a fixed-size list of
[Option<Reg>; 8]to insteadholding
ArrayVec<Reg, 8>in theCastTargettype used whencalculating 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.