Make transpose const and inline #103127
Make transpose const and inline #103127
Conversation
|
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
This comment has been minimized.
This comment has been minimized.
c4a750a to
e87e7e8
Compare
library/core/src/mem/maybe_uninit.rs
Outdated
| @@ -1297,7 +1297,8 @@ impl<T, const N: usize> MaybeUninit<[T; N]> { | |||
| /// let data: [MaybeUninit<u8>; 1000] = MaybeUninit::uninit().transpose(); | |||
| /// ``` | |||
| #[unstable(feature = "maybe_uninit_uninit_array_transpose", issue = "96097")] | |||
| pub fn transpose(self) -> [MaybeUninit<T>; N] { | |||
| #[inline(always)] | |||
There was a problem hiding this comment.
If you make these just #[inline] instead of #[inline(always)], I'll approve it right away.
But for always you need to make a case that it actually matters to have the stronger version.
There was a problem hiding this comment.
I guess I don't understand when #[inline] wouldn't fire vs. #[inline(always)]. As for why the stronger version matters, if the compiler ever decides not to inline we end up with a memcpy whereas inlining blocks this memcpy.
There was a problem hiding this comment.
Right, the inlining is clearly important. But the main thing that, compared to nothing, #[inline] actually does on generic methods is force it to be available in all CGUs.
Whereas inline-vs-inline(always) in comparison is just the difference between inlinehint and alwaysinline https://llvm.org/docs/LangRef.html#function-attributes, so we generally don't use always. If LLVM really thinks that sometimes it's not worth doing despite the inlinehint, odds are that we should probably let it make that choice. Sometimes we override it, where someone shows that it's worth it, but non-always #[inline] seems sufficient in your above example.
So let's start with non-always, and can always put it there later if needed.
There was a problem hiding this comment.
Right, the inlining is clearly important. But the main thing that, compared to nothing, #[inline] actually does on generic methods is force it to be available in all CGUs.
Interesting, was able to find this. I still don't think I get it, but it kind of seemed like inline leads to a potentially major hit and then always doesn't matter on top of that.
If LLVM really thinks that sometimes it's not worth doing despite the inlinehint, odds are that we should probably let it make that choice.
Except that transpose is supposed to generate no code since it's just a cast. If LLVM doesn't inline, that would probably indicate a bug or some truly big-brain stuff. 😆
Anyway, downgraded.
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
e87e7e8 to
1a1ebb0
Compare
|
@rustbot ready |
|
@bors r+ |
|
⌛ Testing commit 1a1ebb0 with merge fa91a1fe99106e14fa44eacbbfcf76a787afb6db... |
|
💔 Test failed - checks-actions |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@scottmcm can you rerun this? I don't how this failure could be related: |
|
Agreed that that's a super-weird error. @bors retry |
Rollup of 6 pull requests Successful merges: - rust-lang#102863 (Standardize "use parentheses to call" suggestions between typeck and trait selection) - rust-lang#103034 (Let expressions on RHS shouldn't be terminating scopes) - rust-lang#103127 (Make transpose const and inline) - rust-lang#103153 (Allow `Vec::leak` when using `no_global_oom_handling`) - rust-lang#103182 (Clean up query descriptions) - rust-lang#103216 (Consider patterns in fn params in an `Elided(Infer)` lifetime rib.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
r? @scottmcm
self, not inlining would defeat the whole purpose of usingMaybeUninitdue to the copying.