rustc_codegen_llvm: don't overalign loads of pair operands.

[eddyb]

Counterpart to #56300, but for loads instead of stores.

[rust-highfive]

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

[eddyb]

Why is accepted before nominated in the alphabet...

[nikomatsakis]

@bors r+ p=10

[bors]

📌 Commit 51cf4e9 has been approved by nikomatsakis

[nikomatsakis]

cc @rust-lang/compiler

Marking as beta-accepted because small and tight time constraints.

[bors]

⌛ Testing commit 51cf4e9 with merge a49316d...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing a49316d to master...

[nikomatsakis]

Removing beta-accepted so we can discuss in today's @rust-lang/compiler meeting.

[nikic]

I was somewhat skeptical about whether the incorrect alignments could result in actual miscompilations, as usually LLVM cannot do a whole lot with alignments that are larger than the load/store size from a codegen perspective.

I've been able to come up with the following case, which segfaults prior to the alignment fixes:

#[repr(align(16))]
pub struct Foo<T> {
    bar: T,
    baz: T,
}

#[inline(never)]
pub fn test(x: Foo<(i64, i64)>) -> Foo<(i64, i64)> {
    let bar = x.bar;
    let baz = x.baz;
    Foo { bar: (bar.1, baz.0), baz: (0, 0) }
}

fn main() {
    test(Foo { bar: (0, 0), baz: (0, 0) });
}

The trick is to make sure the load of the first pair element is optimized away, while the load for the second pair element is coalesced with adjacent loads into a larger size. This results in codegen emitting movaps instead of movups instructions.

[eddyb]

FWIW this bug already existed AFAICT, I don't know how we hadn't hit it before...

[nikic]

@eddyb Probably due to this code on stable:

rust/src/librustc_codegen_llvm/mir/place.rs

Line 176 in 1433507

let effective_field_align = self.align

This reduces the alignment based on the field alignment, so the alignment of the first pair element ends up being small enough to accomodate both.

[pnkfelix]

discussed in T-compiler meeting. beta-accepting.

[alexcrichton]

@eddyb and/or @nikic, would y'all be able to help out in a backport to beta for this? It appears the patch to src/librustc_codegen_llvm/builder.rs doesn't cleanly apply to beta because the function being patched may not exist? I'm not sure if this affects the beta branch and/or where to apply the fix

[eddyb]

It's PlaceRef::load in mir/place.rs.

[alexcrichton]

Ok thanks! I think this is running into the same errors found on #56300 (comment), can you help out with those?

[alexcrichton]

Ok I think I've figured it out with some help at #56352