rust icon indicating copy to clipboard operation
rust copied to clipboard

Optimization of deinitialization became worse in nightly

Open theemathas opened this issue 1 month ago • 3 comments

Code

I tried this code (not from real code, but just experimenting):

use std::mem::MaybeUninit;

#[unsafe(no_mangle)]
fn demo(x: &mut MaybeUninit<u32>) -> MaybeUninit<u32> {
    *x = MaybeUninit::uninit();
    *x
}

In beta, this compiles to (Godbolt):

demo:
        ret

In nightly, this compiles to (Godbolt):

demo:
        mov     eax, dword ptr [rdi]
        ret

Since we've deinitialized the memory that x points to, returning a copy of that memory should result in returning uninit. The compiler should be free to return whatever garbage data it wants, so just ret is a valid compilation, and is probably the best-performing compilation result.

Version it worked on

It most recently worked on Godbolt full compiler version:

rustc 1.92.0-beta.3 (f3f12444a 2025-11-09)
binary: rustc
commit-hash: f3f12444a017add0468f683f3a10656d29438a05
commit-date: 2025-11-09
host: x86_64-unknown-linux-gnu
release: 1.92.0-beta.3
LLVM version: 21.1.3
Internal compiler ID: beta

Version with regression

Godbolt full compiler version:

rustc 1.93.0-nightly (83e49b75e 2025-12-03)
binary: rustc
commit-hash: 83e49b75e7daf827e4390ae0ccbcb0d0e2c96493
commit-date: 2025-12-03
host: x86_64-unknown-linux-gnu
release: 1.93.0-nightly
LLVM version: 21.1.5
Internal compiler ID: nightly

theemathas avatar Dec 07 '25 03:12 theemathas

Bisected with cargo bisect-rustc --start beta --end main --prompt -- asm and manually inspecting the asm.

searched toolchains 75948c8bb3bd37f1e8ee20273a04edea4c1f84f8 through d427ddfe90367eaa6d2ed7bb8a16559f0230f47a


********************************************************************************
Regression in e9acbd99d384280874129fb7fa0da9faeae0d051
********************************************************************************

Attempting to search unrolled perf builds
ERROR: couldn't find perf build comment

Regressed in #147827. CC @saethlin

theemathas avatar Dec 07 '25 03:12 theemathas

Another similar case:

use std::mem::MaybeUninit;

#[unsafe(no_mangle)]
fn demo(x: &mut MaybeUninit<u32>) {
    *x = MaybeUninit::new(1);
    *x = MaybeUninit::uninit();
}

On beta (Godbolt):

demo:
        ret

On nightly (Godbolt):

demo:
        mov     dword ptr [rdi], 1
        ret

theemathas avatar Dec 07 '25 04:12 theemathas

I think this is fixable if we lower the assignment of the uninit const to a store of an undef scalar. Previously we always lowered to a memcpy of an undef const with type [i8xN], which is how we get into trouble with LLVM's SROA; it sees all these i8 arrays and pointers but with align annotations greater than 1 and tries to produce scalars. We just need to lower to the appropriate scalars. I think.

Note that if you make the type param of the MaybeUninit something that is clearly not a Scalar, these examples produce the bad codegen on stable/beta. I'm not going to worry about those cases.

saethlin avatar Dec 07 '25 15:12 saethlin

The fix that I described above ^ works fine for this "deinitialization" pattern of Scalars, so types like u32. And this is indeed clearly worse on nightly/beta than it is on 1.91.

For types like [u8; 3] which are Aggregates in the backend, I don't see a fix. Those are i24 to LLVM, but cg_ssa doesn't have such a type and it's not clear to me if we'd even want to expose weirdly-sized integers from a codegen backend. For ScalarPair types, we are a little worse off in first example where we should generate just a ret (we now generate 2 movs instead of just 1). There was a clear missed optimization before as well, it's just a bit worse now.

For ScalarPair such as (u8, u16) I've also been unable to find a fix. In the ScalarPair case it looks like the LLVM pass EarlyCSE is handling the second element in the pair but not the first, and that propagates straight through the rest of the opt pipeline, losing the deinitialization of the first element in the pair. I think for Aggregates, we are no worse off than we were.

saethlin avatar Dec 12 '25 23:12 saethlin