rustc_trans: approximate ABI alignment for padding/union fillers.

[eddyb]

Before #45225 and after this PR, unions and enums are filled with integers of size and alignment matching their alignment (e.g. Option<u32> becomes [u32; 2]) instead of mere bytes.
Also, the alignment padding between struct fields gets this treatment after this PR.

Partially helps with some reduced testcases in #46449, although it doesn't solve the bug itself.

[rust-highfive]

r? @estebank

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

[arielb1]

So this needs some tests:

1. a codegen test so we can be aware of our LLVM output - looks important enough
2. perf.rl.o tests for the cases in Nightly rust hangs forever #46449 - both the case that got faster in beta and the case that got slower in beta.

[eddyb]

@bors try

[bors]

⌛ Trying commit 343ac09df10e16ac241e90822b4648f92a5bdf1d with merge e714aa603990186aef4f596bca9e7a63c890d6ef...

[kennytm]

Perf for the reduced test case added to rust-lang/rustc-perf#172 rust-lang/rustc-perf#173.

[bors]

☀️ Test successful - status-travis
State: approved= try=True

[kennytm]

Not completely fixed yet.

Manually comparing the build time of the reduced test case (with futures dependency) on an x86_64-unknown-linux-gnu:

Size	Stable	Nightly	This PR
1024	1.3s	1.2s	1.2s
2048	1.2s	1.7s	1.6s
3072	1.2s	3.4s	3.9s
4096	1.1s	6.6s	5.9s
5120	1.2s	12.7s	10.6s
6144	1.2s	20.1s	17.6s

The numbers are better but still exponential.

Meta

$ rustc +stable -vV
rustc 1.22.1 (05e2e1c41 2017-11-22)
binary: rustc
commit-hash: 05e2e1c41414e8fc73d0f267ea8dab1a3eeeaa99
commit-date: 2017-11-22
host: x86_64-unknown-linux-gnu
release: 1.22.1
LLVM version: 4.0

$ rustc +nightly -vV
rustc 1.24.0-nightly (6fa53b00e 2017-12-09)
binary: rustc
commit-hash: 6fa53b00e7450060a3af9b1ef63169db37e589c2
commit-date: 2017-12-09
host: x86_64-unknown-linux-gnu
release: 1.24.0-nightly
LLVM version: 4.0

$ rustc +try-build -vV
rustc 1.24.0-nightly (e714aa603 2017-12-10)
binary: rustc
commit-hash: e714aa603990186aef4f596bca9e7a63c890d6ef
commit-date: 2017-12-10
host: x86_64-unknown-linux-gnu
release: 1.24.0-nightly
LLVM version: 4.0

[eddyb]

@kennytm Okay, I can confirm that, I was testing with the reduced testcase, do you happen to have something in between? It's not like there should be any cross-crate effects here.

[kennytm]

@eddyb sorry I don’t understand what you do mean by “in between” 😕. I just ran cargo build +stable --release and check the time elapsed.

[eddyb]

@kennytm I meant reduction-wise. I think I can try myself by adding the code of the test to the futures library and go from there.

EDIT: Oh I think one of the loop {}s shortcuts LLVM optimizations.

[eddyb]

@arielb1 I've extended the scope of this PR to interfield padding, and no longer believe it's that relevant to #46449, but because it demonstrates some improvements, I still want to proceed with it.

[ghost]

if wanted == candidate.align(dl).abi() && wanted == candidate.size().bytes()
Don't know if this is causing the breakage but if we are swapping them shouldn't it be:
if wanted == candidate.align(dl).abi() && wanted == candidate.size(d1).bytes().
Aren't we required to have the bytes for both alignment and size equal not just one?

[eddyb]

Added a couple more assertions, although I'd be surprised if they fire at all.

[nagisa]

@bors r+

[bors]

📌 Commit 8a26e04 has been approved by nagisa

[kennytm]

@eddyb Do we still need to backport this?

[eddyb]

@kennytm It brings us back to the situation before #45225, so LLVM shouldn't behave (much) differently, at least on enums that are optimized the same as they were before.

[ghost]

if wanted == candidate.align(dl).abi() && wanted == candidate.size().bytes() not having the size stated seems to be very dangerous. Is there a reason for this or was it just missed?

[eddyb]

@xerofoify I'm sorry but I don't understand what you mean. The code you quote is in a function that hasn't changed behavior at all (was just simplified now that Integer got its own size & align methods).

[ghost]

If that's the case then it's fine. That just seems to be a common mistake I see when refactoring code or fixing a regression. These can cause another regression but if you and I hope state that the refactor is correct it doesn't cause another regression. Not that I don't trust you but it's so common alongside doing double frees on error paths in C/C++ projects I was very skeptical about it.

[eddyb]

@xerofoify I really don't see what could possibly be the problem you're pointing to. The change in that function isn't meant to fix anything or modify its behavior, it's purely aesthetic.

[ghost]

Fine then if it's just that it's OK.

[bors]

⌛ Testing commit 8a26e04 with merge 77efd68...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nagisa
Pushing 77efd68 to master...

[arielb1]

I don't think we want to backport this until we find a solution to #46897

[eddyb]

@arielb1 But this was supposed to improve performance. If only LLVM didn't care about pointees...

[alexcrichton]

Looks like we forgot to backport this for 1.23.0 (sorry about that!) so removing beta tags

[eddyb]

@alexcrichton #45225 was backed out from that beta (IIUC) so there wasn't anything to backport.

[alexcrichton]

Ah ok, great!