Implement trans for the MIR Switch terminator #30337

wesleywiser · Dec 11, 2015

Fixes #29574

rust-highfive · Dec 11, 2015

r? @arielb1

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

wesleywiser · Dec 11, 2015

r? @nikomatsakis

nagisa · Dec 12, 2015

nagisa reviewed Dec 12, 2015

View changes

nagisa · Dec 12, 2015

src/test/run-pass/mir_trans_switch.rs

+        Abc::C => 3,
+        Abc::D => 4,
+        Abc::B(_) => 2,
+        Abc::A(_) => 1,


You need at least a test for the _ => case.

nagisa · Dec 12, 2015

nagisa reviewed Dec 12, 2015

View changes

nagisa · Dec 12, 2015

src/librustc_trans/trans/mir/block.rs

+
+                //Use the diverge block for the default case since all enum variants have targets
+                //and it shouldn't ever be called
+                let diverge_llbb = self.llblock(mir::DIVERGE_BLOCK);


What about the regular default case, i.e. _ =>

nagisa · Dec 12, 2015

nagisa reviewed Dec 12, 2015

View changes

nagisa · Dec 12, 2015

src/librustc_trans/trans/mir/block.rs

+                let discr = adt::trans_get_discr(bcx, &*represented_ty, discr_lvalue.llval, None);
+
+                //Use the diverge block for the default case since all enum variants have targets
+                //and it shouldn't ever be called


NIT: a space after //

dotdash · Dec 12, 2015

dotdash reviewed Dec 12, 2015

View changes

dotdash · Dec 12, 2015

src/librustc_trans/trans/mir/block.rs

+                let discr_lvalue = self.trans_lvalue(bcx, discr);
+                let discr = adt::trans_get_discr(bcx, &*represented_ty, discr_lvalue.llval, None);
+
+                //Use the diverge block for the default case since all enum variants have targets


For exhaustive matches, the default arm of the switch statement should go to a block that only contains an unreachable instruction. LLVM uses that for optimizations.

wesleywiser · Dec 12, 2015

@nagisa Fixed nit and added test case for _ =>. It looks like the default case is translated to all other variants pointing to the same block. (You can see the graphviz for foo2 here: https://gist.github.com/wesleywiser/698dbdd7945ec51a1e3c)

@dotdash I think I fixed this in my last commit. Does that look right to you?

nagisa · Dec 12, 2015

@wesleywiser @dotdash regarding unreachable…

DIVERGE_BLOCK is guaranteed to only contain the Diverge terminator (and no statements), which translates down straight to unreachable. That being said, @wesleywiser, I would remove the b834929

Re _=>, I see, its all good then.

EDIT: ah, right, it might be a Resume in case some llpersonality is set. but it is never set (yet?)

Thinking about that more, resume inside DIVERGE_BLOCK is an unpleasant hack to do, especially in cases like this. There’s some nice properties when you know DIVERGE_BLOCK is always an unreachable. resume is only relevant for function calls which have landing pads, and I believe translation for function calls should take care to generate necessary resume themselves, since they gonna have to generate necessary landingpad insns anyway. Any thoughts on it, @nikomatsakis?

dotdash · Dec 13, 2015

@nagisa AIUI, the fact that the diverge block can contain an unreachable is the hack. The block is meant for unwinding/panicking, but we always translate it, even if there's no way to actually reach it , and only in that case we put the unreachable there. In a perfect world, we wouldn't even create the block in that case.

nagisa · Dec 13, 2015

@dotdash but all you can really do in that block, really, is having a resume, since landing block itself will be dependent on the location where function was generated, no?

I’d rather have DIVERGE_BLOCK be unconditionally unreachable (and translated lazily, only if necessary/removed by CFG simplification), for cases like this (something to branch into after call to diverging function comes into mind as similar case) and a separate Resume terminator which is emitted by librustc_mir/build/scope.rs after all necessary cleanups.

nikomatsakis · Dec 14, 2015

So, it seems to me that perhaps we should just remove DIVERGE_BLOCK altogether, and instead replicate the DIVERGE terminator at the end of every cleanup sequence. What @nagisa says is, I think, true -- that is, that there isn't much useful code there. I'm not sure I included it exactly, probably just by analogy to the "return" block. Certainly, one could also make the argument that RETURN is not needed. I think mainly I was thinking that it's often easier for analysis purposes if you have a fixed set of "postdominator" blocks, but that may not be really true.

nikomatsakis · Dec 14, 2015

As far as having a special block for unreachable code, that seems fine, but it seems like something specific to trans, and not necessarily something that needs to be made manifest in the MIR.

nikomatsakis · Dec 14, 2015

nikomatsakis reviewed Dec 14, 2015

View changes

nikomatsakis · Dec 14, 2015

src/librustc_trans/trans/mir/block.rs

-                unimplemented!()
+            mir::Terminator::Switch { ref discr, ref adt_def, ref targets } => {
+                let adt_ty = bcx.tcx().lookup_item_type(adt_def.did).ty;
+                let represented_ty = adt::represent_type(bcx.ccx(), adt_ty).clone();


Nit: I don't think you need to call clone() here, do you?

nikomatsakis · Dec 14, 2015

nikomatsakis reviewed Dec 14, 2015

View changes

nikomatsakis · Dec 14, 2015

src/librustc_trans/trans/mir/block.rs

+                let represented_ty = adt::represent_type(bcx.ccx(), adt_ty).clone();
+
+                let discr_lvalue = self.trans_lvalue(bcx, discr);
+                let discr = adt::trans_get_discr(bcx, &*represented_ty, discr_lvalue.llval, None);


Nit: &represented_ty should be fine, not &*represented_ty

nikomatsakis · Dec 14, 2015

nikomatsakis reviewed Dec 14, 2015

View changes

nikomatsakis · Dec 14, 2015

src/librustc_trans/trans/mir/block.rs

+                // The else branch of the Switch can't be hit, so branch to an unreachable
+                // instruction so LLVM knows that
+                let unreachable_blk = bcx.fcx.new_temp_block("enum-variant-unreachable");
+                build::Unreachable(unreachable_blk);


As @nagisa says, it might be nice to have just one such block (created lazilly), we could store it in the "MIR trans" state.

nikomatsakis · Dec 14, 2015

nikomatsakis reviewed Dec 14, 2015

View changes

nikomatsakis · Dec 14, 2015

src/librustc_trans/trans/mir/block.rs

+                build::Unreachable(unreachable_blk);
+
+                let switch = build::Switch(bcx, discr, unreachable_blk.llbb, targets.len());
+                for (adt_variant, target) in adt_def.variants.iter().zip(targets) {


Nit: Please add an assert_eq!(adt_def.variants.len(), targets.len())

wesleywiser · Dec 14, 2015

@nikomatsakis Fixed nits.

it might be nice to have just one such block (created lazilly), we could store it in the "MIR trans" state.

Do you want me to work on that in this PR?

nagisa · Dec 14, 2015

You can leave it as it is currently, however, if you can add a FIXME of some sort to that effect.

wesleywiser · Dec 14, 2015

Ok. Should I tidy the branch up too while I'm in there?

nagisa · Dec 14, 2015

Sure!

wesleywiser · Dec 15, 2015

I've squashed all my changes and rebased against master.

nikomatsakis · Dec 15, 2015

@bors r+

nice!

bors · Dec 15, 2015

📌 Commit 2b15361 has been approved by nikomatsakis

bors · Dec 16, 2015

⌛️ Testing commit 2b15361 with merge 073b0f9...

bors · Dec 16, 2015

☀️ 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-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 arielb1 Dec 11, 2015

rust-highfive assigned nikomatsakis and unassigned arielb1 Dec 11, 2015

Implement trans for the MIR Switch terminator
Fixes #29574

Loading status checks…

2b15361

wesleywiser force-pushed the wesleywiser:mir_switch branch from 65231f8 to 2b15361 Dec 15, 2015

bors added a commit that referenced this pull request Dec 16, 2015

Auto merge of #30337 - wesleywiser:mir_switch, r=nikomatsakis
Fixes #29574

073b0f9

bors merged commit 2b15361 into rust-lang:master Dec 16, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

wesleywiser deleted the wesleywiser:mir_switch branch Dec 20, 2015

rust-lang/rust

Join GitHub today

Implement trans for the MIR Switch terminator #30337

Conversation

wesleywiser commented Dec 11, 2015

rust-highfive assigned arielb1 Dec 11, 2015

This comment has been minimized.

rust-highfive commented Dec 11, 2015

This comment has been minimized.

wesleywiser commented Dec 11, 2015

rust-highfive assigned nikomatsakis and unassigned arielb1 Dec 11, 2015

nagisa reviewed Dec 12, 2015 View changes

This comment has been minimized.

nagisa reviewed Dec 12, 2015 View changes

This comment has been minimized.

nagisa reviewed Dec 12, 2015 View changes

This comment has been minimized.

dotdash reviewed Dec 12, 2015 View changes

This comment has been minimized.

This comment has been minimized.

wesleywiser commented Dec 12, 2015

This comment has been minimized.

nagisa commented Dec 12, 2015

This comment has been minimized.

dotdash commented Dec 13, 2015

This comment has been minimized.

nagisa commented Dec 13, 2015

This comment has been minimized.

nikomatsakis commented Dec 14, 2015

This comment has been minimized.

nikomatsakis commented Dec 14, 2015

nikomatsakis reviewed Dec 14, 2015 View changes

This comment has been minimized.

nikomatsakis reviewed Dec 14, 2015 View changes

This comment has been minimized.

nikomatsakis reviewed Dec 14, 2015 View changes

This comment has been minimized.

nikomatsakis reviewed Dec 14, 2015 View changes

This comment has been minimized.

This comment has been minimized.

wesleywiser commented Dec 14, 2015

This comment has been minimized.

nagisa commented Dec 14, 2015

This comment has been minimized.

wesleywiser commented Dec 14, 2015

This comment has been minimized.

nagisa commented Dec 14, 2015

wesleywiser force-pushed the wesleywiser:mir_switch branch from 65231f8 to 2b15361 Dec 15, 2015

This comment has been minimized.

wesleywiser commented Dec 15, 2015

This comment has been minimized.

nikomatsakis commented Dec 15, 2015

This comment has been minimized.

bors commented Dec 15, 2015

bors added a commit that referenced this pull request Dec 16, 2015

This comment has been minimized.

bors commented Dec 16, 2015

This comment has been minimized.

bors commented Dec 16, 2015

Hide details View details bors merged commit 2b15361 into rust-lang:master Dec 16, 2015 2 checks passed

2 checks passed

wesleywiser deleted the wesleywiser:mir_switch branch Dec 20, 2015

nagisa reviewed Dec 12, 2015

View changes

nagisa reviewed Dec 12, 2015

View changes

nagisa reviewed Dec 12, 2015

View changes

dotdash reviewed Dec 12, 2015

View changes

nikomatsakis reviewed Dec 14, 2015

View changes

nikomatsakis reviewed Dec 14, 2015

View changes

nikomatsakis reviewed Dec 14, 2015

View changes

nikomatsakis reviewed Dec 14, 2015

View changes

wesleywiser force-pushed the wesleywiser:mir_switch branch from `65231f8` to `2b15361` Dec 15, 2015

bors merged commit `2b15361` into rust-lang:master Dec 16, 2015
2 checks passed