llvm / llvm/llvm-project

InstCombine removing `undef` store prevents merging of adjacent small-sized stores

Open
#204,430 0 comments 0 reactions 0 assignees View on GitHub
llvm:instcombine
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

this is my first issue, sorry if i get stuff wrong.

given the following rust code ( [godbolt link](https://godbolt.org/z/88Wc7redb) )
```rs
#[repr(C)]
pub struct X {
a : u16,
b : u8,
c : u32
}

pub struct Y(X);
pub struct Z(Y);

#[unsafe(no_mangle)]
pub fn bad(a: &mut X) {
let x = X { a: 0, b: 0, c: 0 };
*a = x;
}

#[unsafe(no_mangle)]
pub fn good(a: &mut Z) {
let x = X { a: 0, b: 0, c: 0 };
*a = Z(Y(x));
}
```
we get the following x86 output :

```x86asm
bad:
mov word ptr [rdi], 0
mov byte ptr [rdi + 2], 0
mov dword ptr [rdi + 4], 0
ret

good:
mov qword ptr [rdi], 0
ret
```

i do not understand exactly why `good` gets optimized as it does, but from what i see it is because rust adds a lot of superfluous stores for each move, which triggers `SROAPass` to make everything i64 operations, which lets following passes easily optimize it to 1 i64 store.

on the other hand, the llvm IR for `bad` starts very basic and straightforward. after `SROAPass`, `InstCombinePass` receives as input

```llvm
define void @bad(ptr noalias nofree noundef align 4 dereferenceable(8) %a) unnamed_addr {
start:
store i16 0, ptr %a, align 4
%x.sroa.4.0.a.sroa_idx = getelementptr inbounds i8, ptr %a, i64 2
store i8 0, ptr %x.sroa.4.0.a.sroa_idx, align 2
%x.sroa.5.0.a.sroa_idx = getelementptr inbounds i8, ptr %a, i64 3
store i8 undef, ptr %x.sroa.5.0.a.sroa_idx, align 1
%x.sroa.51.0.a.sroa_idx = getelementptr inbounds i8, ptr %a, i64 4
store i32 0, ptr %x.sroa.51.0.a.sroa_idx, align 4
ret void
}
```

and transforms it into
```llvm
define void @bad(ptr noalias nofree noundef align 4 dereferenceable(8) %a) unnamed_addr {
start:
store i16 0, ptr %a, align 4
%x.sroa.4.0.a.sroa_idx = getelementptr inbounds i8, ptr %a, i64 2
store i8 0, ptr %x.sroa.4.0.a.sroa_idx, align 2
%x.sroa.51.0.a.sroa_idx = getelementptr inbounds i8, ptr %a, i64 4
store i32 0, ptr %x.sroa.51.0.a.sroa_idx, align 4
ret void
}
```
the input was optimizable into 1 i64 store, but the output isn't, which is why it seems to me that `InstCombinePass` is at fault.
i have been told though that even if it did not remove the store, there are no passes that would have folded all the stores into one atm.

rust issue : https://github.com/rust-lang/rust/issues/157373

other rust issue from 2024 with the same problem : https://github.com/rust-lang/rust/issues/122274

Contributor guide

Open the contributing guide

Research direction

Start with the SROAPass output and the InstCombinePass input/output shown in the report. Trace why InstCombine removes the undef store and inspect existing InstCombine or store-merging regression coverage; the issue is resolved when the adjacent small stores remain optimizable and a regression test captures the behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
compilers, performance
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.