[MIR] Refactor the MIR translator to use LLVM Builder directly #31282

pczarn · 2016-01-29T12:36:24Z

nagisa · 2016-01-29T12:47:58Z

src/librustc_trans/trans/common.rs

+        // Create a fresh builder from the crate context.
+        let builder = self.ccx().builder();
+        // Set the builder's position to this block's end.
+        builder.position_at_end(self.llbb);


One of the points (other than getting rid of the unnecessary checks) was to get rid of repositioning the builder for every instruction. This should be not necessary (not in MIR, at least). I think it would be better to have something like

struct BlockBuilder<...> { builder: Builder<...> bcx: Block<...> } // split out the `Builder` trait from the `Builder` struct in trans/builder, and… impl<...> Builder for BlockBuilder<...> { // has all the functions that produce LLVM insns (likely generated by a macro or something) }

so one could just call

bcx.br(self.llblock(target))

anywhere in the MIR, instead of doing the B(bcx).br(...) or bcx.build().br(...) dance every time we emit an instruction.

In such (or similar) grand scheme of things each block would have an associated builder and no repositions would be necessary.

Each crate has only one builder, which BuilderRef points to. I'm worried the lack of repositioning could introduce bugs. For example, after the call to MirContext.unreachable_block which may emit an instruction, the builder for bcx must be repositioned (I think).

Also, keeping builder refs alongside blocks in the table would add at least several bytes of overhead for each block. Otherwise, you'd need a different dance every time bcx is passed from MIR to common trans.

Each crate has only one builder, which BuilderRef points to.

You can create LLVM builders at will. The BuilderRef in LocalCrateContext is just a cached builder for the purposes of the old translator. No need to restrict ourselves only to the things available to the old translator.

I'm worried the lack of repositioning could introduce bugs.

True, especially when there could be two different builders acting on the same basic block (i.e. bcx is passed to some build::* function), but the bugs are easily preventable by making bcx in BlockBuilder private and adding a method similar to this:

fn with_old_bcx<R, F: FnOnce(b: Block<...>) -> R> (&self, f: F) { f(self.bcx); self.builder.position_at_end(self.bcx.llbb); }

Also, keeping builder refs alongside blocks in the table would add at least several bytes of overhead for each block.

I don’t find that to be a problem. At any given point the translator is dealing with a small amount of blocks. I don’t think there’s any place in MIR trans where it would be necessary to have more than 2 or 3 BlockBuilders alive at any given time. If you are worried about allocation overhead, dropped builders could be reused in some way as well.

Otherwise, you'd need a different dance every time bcx is passed from MIR to common trans.

bulder.with_old_bcx(|bcx| some_old_trans_function_wants_bcx(bcx, ...)) is much more trivial than sprinkling a bcx.builder() everywhere, where MIR trans wants to emit an instruction. Consider, that the old trans will be phased out in the future and we should adapt (once the old trans is gone) the common trans utilities to match the MIR trans model, and not the other way around.

All in all, while this PR indeed makes MIR’s use of LLVM builder more direct, in my opinion there’s much larger refactoring opportunities available. If you are not interested in exploring them, I’ll r+ this provided you change all bcx.build() to be B(bcx) (where B == build::B), because it does essentially the same thing as this method, but is shorter to type out and already exists.

bors · 2016-01-30T03:18:25Z

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

nagisa · 2016-01-30T23:31:23Z

src/librustc_trans/trans/common.rs

+    }
+}
+
+impl<'blk, 'tcx> Deref for BlockAndBuilder<'blk, 'tcx> {


nagisa · 2016-01-31T00:01:43Z

I really like what this is looking like. I do not find lack of builder recycling to be a big problem or a blocker to land this (rather, that would be a premature optimisation, and builder construction seems to be pretty cheap anyway)

I’ll let this stay around for a day or two so @nikomatsakis gets a chance to see it, but otherwise r=me with the two nits (and the tidy failure) fixed.

pczarn · 2016-02-01T10:17:23Z

Finished. I have some thoughts: almost all methods in the mir directory could be implemented for Block rather than MirContext.

Storing LandingPads directly in blocks made them bigger. I changed that. When MIR finally replaces trans, we should check how much we can save by making blocks smaller.

nagisa · 2016-02-01T10:39:12Z

src/librustc_trans/trans/base.rs

@@ -1616,6 +1616,7 @@ pub fn new_fn_ctxt<'a, 'tcx>(ccx: &'a CrateContext<'a, 'tcx>,
        param_substs: param_substs,
        span: sp,
        block_arena: block_arena,
+        lpad_arena: TypedArena::new(),


You might want to leave off this change for a separate PR, honestly (or at least factor it out into a separate commit, so it easier to see related changes).

Code related to this field is very precondition-sensitive, I’m worried changing it like this might invalidate some of those preconditions. E.g. MSVC landing blocks, especially, are on per-block basis and it is likely to be wrong there.

Ah, I see it, you’re still storing a pointer inside a Block. A few thoughts:

Size of BlockS does not really matter, because we only ever pass a Block around, which is a pub type Block<'blk, 'tcx> = &'blk BlockS<'blk, 'tcx>; – i.e. is a reference and there’s barely any performance overhead passing a pointer to larger or smaller struct;

From my experimentation with implementing MSVC LPs in MIR, all that really needs to be stored around for MIR itself is a single ValueRef (a pointer, basically) inside MirContext. All the landing-pad related things in Block are for the old trans to use, and I wouldn’t bother much changing it (but since you’ve already changed it, its fine, I guess?).

I’d still like it split up into a separate commit so it is easier to review and these changes do not mix with the Builder refactor.

nagisa · 2016-02-01T11:47:45Z

almost all methods in the mir directory could be implemented for Block rather than MirContext

It doesn’t make much sense, does it? All the methods operate strictly on MIR, don’t they. It certainly might be a case that mir directory contents will get lifted up into trans over time, though.

nagisa · 2016-02-01T12:03:47Z

src/librustc_trans/trans/mir/operand.rs

                         lldest: ValueRef,
                         operand: OperandRef<'tcx>)
    {
        debug!("store_operand: operand={}", operand.repr(bcx));
+        bcx.with_block(|bcx| {
+            self.store_operand_direct(bcx, lldest, operand)


Why not inline?

bors · 2016-02-06T09:24:37Z

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

pczarn · 2016-02-08T11:17:33Z

Updated.

nagisa · 2016-02-08T11:58:08Z

src/librustc_trans/trans/mir/mod.rs

+               let lltemp = bcx.with_block(|bcx| {
+                   base::alloc_ty(bcx, arg_ty, &format!("arg{}", arg_index))
+               });
+               let (dataptr, meta) = bcx.with_block(|bcx| {


Merge into with_block above and return a (lltemp, dataptr, meta)?

nagisa · 2016-02-08T11:58:16Z

src/librustc_trans/trans/mir/mod.rs

-               base::store_ty(bcx, llarg, lltemp, arg_ty);
+               let lltemp = bcx.with_block(|bcx| {
+                   base::alloc_ty(bcx, arg_ty, &format!("arg{}", arg_index))
+               });


nagisa · 2016-02-08T12:00:08Z

Please squash the f \d commits into one with a meaningful description. r=me once the last two nits are fixed.

pczarn · 2016-02-08T21:37:46Z

Finished.

* We don't have SEH-based unwinding yet. For this reason we don't need operand bundles in MIR trans. * Refactored some uses of fcx. * Refactored some calls to `with_block`.

nagisa · 2016-02-09T08:39:20Z

@bors r+

bors · 2016-02-09T08:39:20Z

📌 Commit 38fa06b has been approved by nagisa

bors · 2016-02-09T08:50:04Z

⌛ Testing commit 38fa06b with merge 8b95b0a...

Closes #31003

bors · 2016-02-09T10:50:01Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

rust-highfive assigned nagisa Jan 29, 2016

pczarn force-pushed the mir-trans-builder branch from 270ebcd to 4b44082 Compare January 29, 2016 12:38

nagisa reviewed Jan 29, 2016
View reviewed changes

pczarn force-pushed the mir-trans-builder branch from 3646783 to 81f8a28 Compare January 30, 2016 23:17

nagisa reviewed Jan 30, 2016
View reviewed changes

src/librustc_trans/trans/common.rs

}

}

impl<'blk, 'tcx> Deref for BlockAndBuilder<'blk, 'tcx> {

Copy link

Member

nagisa Jan 30, 2016

Choose a reason for hiding this comment

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

Nice!

pczarn force-pushed the mir-trans-builder branch from 81f8a28 to fb2f547 Compare February 1, 2016 10:06

nagisa reviewed Feb 1, 2016
View reviewed changes

pczarn added 3 commits February 8, 2016 11:12

Implement OwnedBuilder and BlockAndBuilder

8b77683

Refactor the MIR translator to use LLVM Builder directly

06266eb

Refactor storage of LandingPads

a9ab809

pczarn force-pushed the mir-trans-builder branch 2 times, most recently from 83c7825 to 8c19204 Compare February 8, 2016 11:01

pczarn force-pushed the mir-trans-builder branch from 8c19204 to 5949393 Compare February 8, 2016 11:22

nagisa reviewed Feb 8, 2016
View reviewed changes

pczarn force-pushed the mir-trans-builder branch from 5949393 to 5d67d76 Compare February 8, 2016 21:35

Cleanup based on review by @nagisa

38fa06b

* We don't have SEH-based unwinding yet. For this reason we don't need operand bundles in MIR trans. * Refactored some uses of fcx. * Refactored some calls to `with_block`.

pczarn force-pushed the mir-trans-builder branch from 5d67d76 to 38fa06b Compare February 8, 2016 22:08

bors added a commit that referenced this pull request Feb 9, 2016

Auto merge of #31282 - pczarn:mir-trans-builder, r=nagisa

8b95b0a

Closes #31003

bors merged commit 38fa06b into rust-lang:master Feb 9, 2016

This was referenced Feb 9, 2016

std: Move pc-windows-gnu to SEH-based unwinding #31313

Closed

[MIR] Some initial zeroing implementation work #31430

Merged

[MIR] implement zero-on-move #31449

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MIR] Refactor the MIR translator to use LLVM Builder directly #31282

[MIR] Refactor the MIR translator to use LLVM Builder directly #31282

pczarn commented Jan 29, 2016

nagisa Jan 29, 2016

pczarn Jan 29, 2016

nagisa Jan 29, 2016

bors commented Jan 30, 2016

nagisa Jan 30, 2016

nagisa commented Jan 31, 2016

pczarn commented Feb 1, 2016

nagisa Feb 1, 2016

nagisa Feb 1, 2016

nagisa commented Feb 1, 2016

nagisa Feb 1, 2016

bors commented Feb 6, 2016

pczarn commented Feb 8, 2016

nagisa Feb 8, 2016

nagisa Feb 8, 2016

nagisa commented Feb 8, 2016

pczarn commented Feb 8, 2016

nagisa commented Feb 9, 2016

bors commented Feb 9, 2016

bors commented Feb 9, 2016

bors commented Feb 9, 2016

[MIR] Refactor the MIR translator to use LLVM Builder directly #31282

[MIR] Refactor the MIR translator to use LLVM Builder directly #31282

Conversation

pczarn commented Jan 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jan 30, 2016

Choose a reason for hiding this comment

nagisa commented Jan 31, 2016

pczarn commented Feb 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa commented Feb 1, 2016

Choose a reason for hiding this comment

bors commented Feb 6, 2016

pczarn commented Feb 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa commented Feb 8, 2016

pczarn commented Feb 8, 2016

nagisa commented Feb 9, 2016

bors commented Feb 9, 2016

bors commented Feb 9, 2016

bors commented Feb 9, 2016