Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Partially-undef constants cause a large size regression in src/test/run-make/wasm-panic-small #84565

Closed
erikdesjardins opened this issue Apr 25, 2021 · 1 comment · Fixed by #94130
Closed

Partially-undef constants cause a large size regression in src/test/run-make/wasm-panic-small #84565

erikdesjardins opened this issue Apr 25, 2021 · 1 comment · Fixed by #94130
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. I-heavy Issue: Problems and improvements with respect to binary size of generated code.

Comments

@erikdesjardins
Copy link
Contributor

erikdesjardins commented Apr 25, 2021 •
edited
Loading

Opening this to track a limitation of #83698, which does not generate partially-undef constants (controlled by -Zpartially-uninit-const-threshold=1234) due to this issue.

The root cause is that LLVM is unable to const prop (?) a load from a partially-undef constant, which prevents all the panic formatting code from being removed in the src/test/run-make/wasm-panic-small test, which causes the test to fail.

Minimal repro (from #83698 (comment)): https://godbolt.org/z/3vYqM5865

per nikic:

https://godbolt.org/z/1Mff6zrvd shows one of the ways this could be fixed on the LLVM side. We should be dropping trailing zeros from bitcasted GEPs.
@rustbot rustbot added I-heavy Issue: Problems and improvements with respect to binary size of generated code. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ labels Apr 25, 2021
@erikdesjardins erikdesjardins changed the title Undef in constants causes a large size regression in src/test/run-make/wasm-panic-small Partially-undef constants cause a large size regression in src/test/run-make/wasm-panic-small Apr 28, 2021
@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. and removed O-wasm Target: WASM (WebAssembly), http://webassembly.org/ labels Apr 28, 2021
@erikdesjardins erikdesjardins mentioned this issue Aug 7, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 26, 2021
@bors
Auto merge of rust-lang#83698 - erikdesjardins:undefconst, r=RalfJung…
20997f6 
…,oli-obk

Use undef for uninitialized bytes in constants

Fixes rust-lang#83657

This generates good code when the const is fully uninit, e.g.

```rust
#[no_mangle]
pub const fn fully_uninit() -> MaybeUninit<[u8; 10]> {
    const M: MaybeUninit<[u8; 10]> = MaybeUninit::uninit();
    M
}
```
generates
```asm
fully_uninit:
	ret
```

as you would expect.

There is no improvement, however, when it's partially uninit, e.g.

```rust
pub struct PartiallyUninit {
    x: u64,
    y: MaybeUninit<[u8; 10]>
}

#[no_mangle]
pub const fn partially_uninit() -> PartiallyUninit {
    const X: PartiallyUninit = PartiallyUninit { x: 0xdeadbeefcafe, y: MaybeUninit::uninit() };
    X
}
```
generates
```asm
partially_uninit:
	mov	rax, rdi
	mov	rcx, qword ptr [rip + .L__unnamed_1+16]
	mov	qword ptr [rdi + 16], rcx
	movups	xmm0, xmmword ptr [rip + .L__unnamed_1]
	movups	xmmword ptr [rdi], xmm0
	ret

.L__unnamed_1:
	.asciz	"\376\312\357\276\255\336\000"
	.zero	16
	.size	.L__unnamed_1, 24
```
which copies a bunch of zeros in place of the undef bytes, the same as before this change.

Edit: generating partially-undef constants isn't viable at the moment anyways due to rust-lang#84565, so it's disabled
@erikdesjardins
Copy link
Contributor Author

Underlying issue should be fixed in the next LLVM release by https://reviews.llvm.org/D111023. Thanks @nikic!

When LLVM 14 is out, I will revisit enabling this.

@erikdesjardins erikdesjardins mentioned this issue Feb 18, 2022
Merged
@bors bors closed this as completed in ece55d4 Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. I-heavy Issue: Problems and improvements with respect to binary size of generated code.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use undef for (some) partially-uninit constants erikdesjardins/rust
2 participants
@erikdesjardins @rustbot