Repository navigation
Weird copy happening during initialization #56191
Description
This code:
#[inline(never)]
pub fn do_ants() {
do_item(&Outer {
item: SpecificDisplayItem2::PopStackingContext,
info: LayoutPrimitiveInfo2::new(),
});
}
#[derive(Copy, Clone)]
pub struct LayoutPrimitiveInfo2 {
pub rect: [f32; 7],
pub is_backface_visible: u16,
}
impl LayoutPrimitiveInfo2 {
fn new() -> Self {
Self {
rect: [0.; 7],
is_backface_visible:0,
}
}
}
pub enum SpecificDisplayItem2 {
PopStackingContext,
Other([f64; 22]),
}
struct Outer {
item: SpecificDisplayItem2,
info: LayoutPrimitiveInfo2
}
fn do_item(di: &Outer) { unsafe { ext(di) } }
extern {
fn ext(di: &Outer);
}compiles to
bad_pop_stacking_context::do_ants:
mov rbp, rsp
sub rsp, 256
xorps xmm0, xmm0
movaps xmmword, ptr, [rbp, -, 32], xmm0
mov qword, ptr, [rbp, -, 10], 0
mov qword, ptr, [rbp, -, 16], 0
mov qword, ptr, [rbp, -, 248], 0
mov rax, qword, ptr, [rbp, -, 32]
mov rcx, qword, ptr, [rbp, -, 24]
mov qword, ptr, [rbp, -, 64], rax
mov qword, ptr, [rbp, -, 56], rcx
mov rax, qword, ptr, [rbp, -, 16]
mov qword, ptr, [rbp, -, 48], rax
mov rax, qword, ptr, [rbp, -, 8]
mov qword, ptr, [rbp, -, 40], rax
lea rdi, [rbp, -, 248]
call _ext
add rsp, 256
pop rbp
retWhen the u16 is changed to a u32 we get the saner:
bad_pop_stacking_context::do_ants:
mov rbp, rsp
sub rsp, 224
mov qword, ptr, [rbp, -, 216], 0
mov qword, ptr, [rbp, -, 8], 0
mov qword, ptr, [rbp, -, 16], 0
mov qword, ptr, [rbp, -, 24], 0
mov qword, ptr, [rbp, -, 32], 0
lea rdi, [rbp, -, 216]
call _ext
add rsp, 224
pop rbp
ret
Activity
eddyb commentedon Nov 24, 2018
You should be able to reproduce any of these with C++ code in
clangand report them as LLVM bugs.But also, it helps to explain what is weird here. In this case, my guess is the reads back out from the stack? Also, we should ignore the lack of SSE usage in the saner example, at least for now, right?
cc @rust-lang/wg-codegen Can someone look at the LLVM IR here and try to figure out if there's anything we could tell LLVM that would make it not do this?
nagisa commentedon Nov 24, 2018
The code we generate for the structure and its initalization looks something like this:
this
undefseems to be the thing that prevents proper optimisation from taking place. Explicitly adding intialization of padding to 0:makes GVN generate a memset for the whole structure, rather than a part of it, which then makes instcombine happier and prone to optimizing out stuff better.
eddyb commentedon Nov 24, 2018
Hmm, can we indicate to LLVM that it could put anything in the padding?
I'd rather not initialize padding ourselves since that can be a deoptimization.
nagisa commentedon Nov 24, 2018
EDIT: my bad, was looking at the wrong thing. LLVM is able to reach the code below with
is_backface_visible: u32.Wrong stuff
FWIW we do not reach the "ideal" codegen in either scenario. Ideally the code would look like this
which can be achieved by replacing initialization with
usnafe { ::std::mem::zeroed() }.LLVM already knows (the padding is
undef), but that is exactly what prevents optimizations from kicking in from what I can see. I cannot really tell the exact underlying analysis/optimisation that falls short, and I’m probably not going to spend time on this any time soon.@nikic might be interested, though.
eddyb commentedon Nov 24, 2018
Does it know it could write there, though? That is,
undefvs "value is unmodified".Or is it optimizing all of this on an
alloca?nagisa commentedon Nov 24, 2018
Yes.
nikic commentedon Nov 24, 2018
I don't think that memset formation is the right place to optimize here. In the general case extending a partial memset to a memset of the full structure may not necessarily be advantageous.
It would be preferable to handle this during memset-memcpy forwarding. I have something like https://gist.github.com/nikic/3ca2f08c70bf8d9c0a634d42d5533f17 in mind, which would be enough to cover this case. This will fall short though if there is interior padding.
jrmuizel commentedon Nov 24, 2018
The equivalent c++ generates saner code regardless of the type of is_backface_visible
nikic commentedon Nov 30, 2018
I've submitted https://reviews.llvm.org/D55120.
Ultimately this is just papering things over though. I think a more general solution would be to instead check if a memcpy can be hoisted upwards past a store/memset chain and absorbed into an alloca. Basically what call slot optimization does, just not for calls.
nikic commentedon Dec 8, 2018
Patch landed upstream, so this is waiting on an LLVM update now.
jrmuizel commentedon Dec 8, 2018
nikic commentedon Dec 9, 2018
Yes, I think it would be reasonable to cherry-pick this change. I'd like to wait for a few miscompile fixes to land first, so we can batch them together.
jrmuizel commentedon Dec 27, 2018
@nikic the llvm patch has baked for a while now. Do you think you could cherry pick it into rust's llvm?
jrmuizel commentedon Jan 27, 2019
Fixed by #57675
We now get: