Cleanup old trans #38302

Mark-Simulacrum · 2016-12-11T17:42:15Z

This is a cleanup of old trans, with the following main points:

Remove the build.rs API (prefer using Builder directly, which is now passed where needed through BlockAndBuilder).
Remove Block (inlining it into BlockAndBuilder)
Remove Callee::call, primarily through inlining and simplification of code.
Thinned FunctionContext:
- mir, debug_scopes, scopes, and fn_ty are moved to MirContext.
- param_env is moved to SharedCrateContext and renamed to empty_param_env.
- llretslotptr is removed, replaced with more careful management of the return values in calls.
- landingpad_alloca is inlined into cleanup.
- param_substs are moved to MirContext.
- span is removed, it was never set to anything but None.
- block_arena and lpad_arena are removed, since neither was necessary (landing pads and block are quite small, and neither needs arena allocation).
Fixed drop_in_place not running other destructors in the same function.

Fixes #35566 (thanks to @est31 for confirming).

arielb1 · 2016-12-11T18:14:31Z

Nice! You don't have to wait for my shim-MIR PR - I can handle that.

hanna-kruppe · 2016-12-11T19:09:28Z

src/librustc_trans/adt.rs

IIUC this function is redundant now. Is cleaning that up in scope for this PR?

Yeah, I also commented on it, there's a bunch like this.

nagisa · 2016-12-11T19:10:27Z

I will want to look over the PR later.

eddyb

Great work! Most of changes I have in mind are further cleanup (fully removing the global builder and its debuginfo machinery, _builder fn variants, Block etc.).

eddyb · 2016-12-11T18:42:13Z

src/librustc/middle/lang_items.rs

What I meant was actually to add this at the implementation site in liballoc, in case someone changes it.

Good to put this into liballoc. Is this a new requirement, or just a new comment? Since we know the discussion about panic on oom will come back.

If we ever decide to allow allocation to panic, we’ll have to change trans. Not a hard requirement, but not a comment either.

Hmm, actually it looks like it's possible to make this panic friendly; but I may be wrong. @eddyb what we need for that is ownership of BAB, right? We have that, so if that's what we need, I can "fix" this.

You can simply change how MIR for allocation is generated. No need to fiddle in trans… unless there’s some old cruft in old trans that still generates calls to this itself… in which case you’ll have a hard and bad time fixing this :)

No, this is only called from MIR. In fact, the actual function should be inlined.

eddyb · 2016-12-11T18:44:06Z

src/librustc_trans/adt.rs

You can change trans_field_ptr_builder to trans_field_ptr, including its signature. The _builder split was for the old B(.

eddyb · 2016-12-11T18:45:28Z

src/librustc_trans/base.rs

Same thing here, load_ty_builder should not exist anymore.

eddyb · 2016-12-11T18:45:53Z

src/librustc_trans/base.rs

Same _builder cleanup needed.

eddyb · 2016-12-11T18:47:19Z

src/librustc_trans/base.rs

You can probably inline these 2 at their callsites now.

eddyb · 2016-12-11T18:59:22Z

src/librustc_trans/intrinsic.rs

Oh, everywhere you have a call that's not to drop glue or box_free you can just put None instead of bcx.lpad().and_then(|b| b.bundle()) - the latter is for cleanup blocks (unwinding landing pads) which are restricted in MIR, e.g. they can't just have arbitrary function calls like intrinsics.

eddyb · 2016-12-11T19:03:08Z

src/librustc_trans/mir/block.rs

Extraneous space after =.

eddyb · 2016-12-11T19:05:01Z

src/librustc_trans/mir/operand.rs

Rename this function to store_operand.

eddyb · 2016-12-11T19:06:05Z

src/librustc_trans/mir/rvalue.rs

Is this the only call? Because then you could inline it.

eddyb · 2016-12-11T19:08:56Z

src/librustc_trans/tvec.rs

Is this due to over-zealous inference 😞?

est31 · 2016-12-11T19:37:41Z

Let me test whether this fixes issue 35566...

est31 · 2016-12-11T20:11:04Z

Nope, #35566 is still not fixed.

eddyb · 2016-12-11T20:29:26Z

Update: looks like @est31 was hitting the bug at stage0, which ofc is not fixed. I've suggested a manual way to test that the bug is indeed fixed at stage1 and later.

est31 · 2016-12-11T22:16:58Z

Now it fails when compiling stage1 std artifacts:

Basic Block in function '_ZN4core3num7flt2dec15to_shortest_str17hd586f26308963365E' does not have terminator!
label %bb1
LLVM ERROR: Broken function found, compilation aborted!
Build failed, waiting for other jobs to finish...
error: Could not compile `core`.

To learn more, run the command again with --verbose.

bluss · 2016-12-12T16:15:25Z

About the drop_in_place change, is this an example of a program that changes behaviour? Just to understand. Current stable/beta/nightly doesn't run the "B" drop after the panic of "A".

playground

struct PrintDrop(&'static str);

impl Drop for PrintDrop {
    fn drop(&mut self) {
        println!("Dropping {}", self.0);
        if self.0 == "A" {
            panic!();
        }
    }
}

use std::mem::forget;
use std::ptr::drop_in_place;

fn main() {
    let mut x = (PrintDrop("A"), PrintDrop("B"));
    unsafe {
        drop_in_place(&mut x);
        forget(x);
    }
}

Mark-Simulacrum · 2016-12-12T16:45:20Z

The example @eddyb gave me for the drop_in_place change: https://is.gd/i42PKb vs https://is.gd/UfTJjA. The first one panics only once because it uses drop_in_place, while the second panics twice, since it doesn't utilize drop_in_place. I believe your example is equivalent to this, though.

eddyb · 2016-12-16T17:27:24Z

src/librustc_trans/common.rs

You should probably name this alloca_builder.

eddyb · 2016-12-16T17:27:33Z

src/librustc_trans/common.rs

Still need to kill this.

eddyb · 2016-12-16T17:28:52Z

src/librustc_trans/common.rs

I don't think the LLVMGetFirstInstruction call is needed and you can just the result of the .load(...).

The load returns the ValueRef for the first instruction?

The load is the first instruction.

eddyb · 2016-12-16T17:30:15Z

src/librustc_trans/common.rs

Oh this is weird, I wonder if this should maybe be inlined.

eddyb · 2016-12-16T17:30:38Z

src/librustc_trans/common.rs

llty can be computed on this branch alone. Which IMO should not exist anymore.

eddyb · 2016-12-16T17:31:22Z

src/librustc_trans/common.rs

Should rename the method on Builder to dynamic_alloca or something.

eddyb · 2016-12-16T17:32:21Z

src/librustc_trans/context.rs

Looks like the underlying builder is not exactly gone.

Also OwnedBuilder should be the only Builder, i.e. the latter should not be a distinct type.

eddyb · 2016-12-16T17:35:46Z

src/librustc_trans/common.rs

Is this used at all anymore?

eddyb · 2016-12-16T17:36:07Z

src/librustc_trans/common.rs

... or this?

eddyb · 2016-12-16T23:51:10Z

src/librustc_trans/common.rs

Can you avoid having two variables named val, i.e. can you move this up a bit more? Maybe rename it to dummy.

eddyb · 2016-12-17T19:22:25Z

src/librustc_trans/debuginfo/create_scope_map.rs

Can this be a regular reference?

eddyb · 2016-12-17T19:28:19Z

src/librustc_trans/mir/mod.rs

Can this get away with being a reference?

eddyb · 2016-12-18T02:23:39Z

src/librustc_trans/adt.rs

Indentation is off here.

eddyb · 2016-12-18T02:29:25Z

src/librustc_trans/base.rs

This is just an alias for build_return_block and the latter should be inlined.
In some cases there is no return value, in the rest the ABI should match so it's just a matter of passing a return pointer in where applicable and returning the result of the call elsewhere.

Looked again and I think trans_ctor_shim is the only place that needs an alloca for the return value, sometimes (and then only a few cases in build_return_block apply).

eddyb · 2016-12-18T02:30:09Z

src/librustc_trans/base.rs

Yeah, all of this should be unnecessary, it'd be a waste of time to actually write something to an alloca and read it later.

eddyb · 2016-12-18T02:36:40Z

src/librustc_trans/callee.rs

Something tells me all remaining uses have None for lpad. Also, trans_call_inner can be inlined.

Wait, that's poorly named, it should be cleanup_bundle.

eddyb · 2016-12-18T02:39:08Z

src/librustc_trans/callee.rs

This comment is outdated heh.

eddyb · 2016-12-18T02:53:39Z

src/librustc_trans/intrinsic.rs

I think this can be folded into the get_simple_intrinsic use below.

eddyb · 2016-12-18T03:01:55Z

src/librustc_trans/mir/block.rs

Needs to be ret_bcx.set_source_location(...).

eddyb · 2016-12-18T03:04:57Z

src/librustc_trans/mir/block.rs

This function can probably just iterate over cleanup_kinds to produce funclets, without the RPO visit order (as CleanupKind::Internal doesn't need to reference a different functlets entry).

eddyb · 2016-12-18T03:06:52Z

src/librustc_trans/mir/mod.rs

This is what I mean, the order is irrelevant here now.

eddyb · 2016-12-18T03:10:13Z

src/librustc_trans/tvec.rs

This is mildly confusing, heh, it'd make more sense as header_bcx.add_incoming_to_phi(...).

bors · 2016-12-18T11:21:13Z

☔ The latest upstream changes (presumably #37429) made this pull request unmergeable. Please resolve the merge conflicts.

eddyb · 2016-12-19T02:40:57Z

src/librustc_trans/glue.rs

This can't ever be the case.

eddyb · 2016-12-19T03:01:47Z

src/librustc_trans/debuginfo/source_loc.rs

FWIW, here and a few times above, builder has a ccx of its own. trans::debuginfo should maybe take just debug_context, not the whole mir context.

eddyb · 2016-12-19T03:05:42Z

src/librustc_trans/mir/mod.rs

Yeah I'd rather these not be made public.

This does not make unreachable and other terminators take self by-value because it is deemed too difficult. We would need to create by-value methods on BAB that call into Builder, due to the Deref to builder.

We cannot inline due to it being recursive.

Also improves cache quality.

Mark-Simulacrum · 2016-12-21T03:37:46Z

Rebased.

eddyb · 2016-12-21T03:38:46Z

@bors r+

bors · 2016-12-21T03:38:46Z

📌 Commit 0013d4c has been approved by eddyb

bors · 2016-12-21T10:38:20Z

⌛ Testing commit 0013d4c with merge 1b38776...

@est31

Cleanup old trans This is a cleanup of old trans, with the following main points: - Remove the `build.rs` API (prefer using `Builder` directly, which is now passed where needed through `BlockAndBuilder`). - Remove `Block` (inlining it into `BlockAndBuilder`) - Remove `Callee::call`, primarily through inlining and simplification of code. - Thinned `FunctionContext`: - `mir`, `debug_scopes`, `scopes`, and `fn_ty` are moved to `MirContext`. - `param_env` is moved to `SharedCrateContext` and renamed to `empty_param_env`. - `llretslotptr` is removed, replaced with more careful management of the return values in calls. - `landingpad_alloca` is inlined into cleanup. - `param_substs` are moved to `MirContext`. - `span` is removed, it was never set to anything but `None`. - `block_arena` and `lpad_arena` are removed, since neither was necessary (landing pads and block are quite small, and neither needs arena allocation). - Fixed `drop_in_place` not running other destructors in the same function. Fixes #35566 (thanks to @est31 for confirming).

bors · 2016-12-21T13:59:10Z

rust-highfive assigned eddyb Dec 11, 2016

hanna-kruppe reviewed Dec 11, 2016

View reviewed changes

eddyb suggested changes Dec 11, 2016

View reviewed changes

Mark-Simulacrum force-pushed the trans-cleanup branch from d87189d to a81bc72 Compare December 12, 2016 00:38

eddyb suggested changes Dec 16, 2016

View reviewed changes

Mark-Simulacrum force-pushed the trans-cleanup branch 3 times, most recently from f02257a to bcfe02d Compare December 16, 2016 21:02

eddyb reviewed Dec 16, 2016

View reviewed changes

Mark-Simulacrum force-pushed the trans-cleanup branch 2 times, most recently from 5a2f7d8 to 11fb0a1 Compare December 17, 2016 03:09

Mark-Simulacrum changed the title ~~Remove build.rs from trans~~ Cleanup old trans Dec 17, 2016

eddyb reviewed Dec 17, 2016

View reviewed changes

src/librustc_trans/debuginfo/create_scope_map.rs Outdated

Copy link

Member

eddyb Dec 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be a regular reference?

eddyb reviewed Dec 17, 2016

View reviewed changes

src/librustc_trans/mir/mod.rs Outdated

Copy link

Member

eddyb Dec 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this get away with being a reference?

Mark-Simulacrum force-pushed the trans-cleanup branch from 832e1e4 to ff3829d Compare December 17, 2016 20:05

eddyb reviewed Dec 18, 2016

View reviewed changes

Mark-Simulacrum mentioned this pull request Dec 19, 2016

Properly fix the personality_fn setting in MIR trans #38462

Closed

Mark-Simulacrum force-pushed the trans-cleanup branch from d4f0717 to aa9b367 Compare December 19, 2016 02:13

eddyb reviewed Dec 19, 2016

View reviewed changes

src/librustc_trans/glue.rs Outdated

Copy link

Member

eddyb Dec 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can't ever be the case.

eddyb reviewed Dec 19, 2016

View reviewed changes

src/librustc_trans/mir/mod.rs Outdated

Copy link

Member

eddyb Dec 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'd rather these not be made public.

Mark-Simulacrum added 21 commits December 20, 2016 20:04

Replace bcx.ccx() with bcx.ccx

88202c5

Mutate llargs instead of reconstructing it.

15c9e5e

Change *.fcx.ccx to *.ccx

6fac0a1

Do not use BAB after calling unreachable.

d55e739

This does not make unreachable and other terminators take self by-value because it is deemed too difficult. We would need to create by-value methods on BAB that call into Builder, due to the Deref to builder.

Use fn_ty directly

6f368e6

Move get_landing_pad onto DropVal.

0d5a8ad

Remove needless check

6a1ec55

Inline make_drop_glue

b9f1064

Remove outdated comment

98a13ff

Reduce coerce_unsized_into to one call

295ea0d

We cannot inline due to it being recursive.

slice_for_each gives a reference already

15b9b27

Remove fn_ty from FunctionContext

bd009dc

Remove outdated comment

3198797

Move eh_personality() onto CrateContext

57914f6

Simplify callee by removing is_indirect branch.

07cf2a9

Add unreachable() after calls to eh_unwind_resume.

654131c

Simplify get_landing_pad by inlining UnwindKind.

a811f60

Move eh_unwind_resume into CrateContext

a9b5c63

Also improves cache quality.

Improve cache quality for eh_personality.

c1bc5e5

Reuse cleanup pad declared at start of block.

079abd0

Fix rebase errors.

0013d4c

Mark-Simulacrum force-pushed the trans-cleanup branch from 7d03df9 to 0013d4c Compare December 21, 2016 03:37

bors merged commit 0013d4c into rust-lang:master Dec 21, 2016

Mark-Simulacrum deleted the trans-cleanup branch December 27, 2016 00:26

Uh oh!

Cleanup old trans #38302

Cleanup old trans #38302

Uh oh!

Conversation

Mark-Simulacrum commented Dec 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arielb1 commented Dec 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nagisa commented Dec 11, 2016

Uh oh!

eddyb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nagisa Dec 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb Dec 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

est31 commented Dec 11, 2016

Uh oh!

est31 commented Dec 11, 2016

Uh oh!

eddyb commented Dec 11, 2016

Uh oh!

est31 commented Dec 11, 2016

Uh oh!

bluss commented Dec 12, 2016

Uh oh!

Mark-Simulacrum commented Dec 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Mark-Simulacrum commented Dec 11, 2016 •

edited

Loading

nagisa Dec 12, 2016 •

edited

Loading

eddyb Dec 11, 2016 •

edited

Loading