MIR: Don't generate 3-armed boolean switch from match. by luqmana · Pull Request #33583 · rust-lang/rust

luqmana · 2016-05-12T09:06:13Z

Snippet from issue:

fnfoo(x:bool,y:bool) -> u32{match(x, y){(false, _) => 0,(_,false) => 1,(true,true) => 2,}}

Generated MIR:

fn foo(arg0: bool, arg1: bool) -> u32 {
let var0: bool; // "x" in scope 1 at 3bbm.rs:17:8: 17:9
let var1: bool; // "y" in scope 1 at 3bbm.rs:17:17: 17:18
let mut tmp0: (bool, bool);
let mut tmp1: bool;
let mut tmp2: bool;
let mut tmp3: (&'static str, &'static str, u32);
let mut tmp4: &'static (&'static str, &'static str, u32);
bb0: {
var0 = arg0; // scope 1 at 3bbm.rs:17:8: 17:9
var1 = arg1; // scope 1 at 3bbm.rs:17:17: 17:18
tmp1 = var0; // scope 5 at 3bbm.rs:18:12: 18:13
tmp2 = var1; // scope 6 at 3bbm.rs:18:15: 18:16
tmp0 = (tmp1, tmp2); // scope 4 at 3bbm.rs:18:11: 18:17
if((tmp0.0: bool)) -> [true: bb4, false: bb1]; // scope 3 at 3bbm.rs:19:10: 19:15
}
bb1: {
return = const 0u32; // scope 10 at 3bbm.rs:19:23: 19:24
goto -> bb7; // scope 3 at 3bbm.rs:18:5: 22:6
}
bb2: {
return = const 1u32; // scope 11 at 3bbm.rs:20:23: 20:24
goto -> bb7; // scope 3 at 3bbm.rs:18:5: 22:6
}
bb3: {
return = const 2u32; // scope 12 at 3bbm.rs:21:25: 21:26
goto -> bb7; // scope 3 at 3bbm.rs:18:5: 22:6
}
bb4: {
if((tmp0.1: bool)) -> [true: bb5, false: bb2]; // scope 3 at 3bbm.rs:20:13: 20:18
}
bb5: {
if((tmp0.0: bool)) -> [true: bb3, false: bb6]; // scope 3 at 3bbm.rs:21:10: 21:14
}
bb6: {
tmp4 = promoted0; // scope 3 at 3bbm.rs:18:5: 22:6
core::panicking::panic(tmp4); // scope 3 at 3bbm.rs:18:5: 22:6
}
bb7: {
return; // scope 0 at 3bbm.rs:17:1: 23:2
}
}

Not sure about this approach. I was also thinking maybe just a standalone pass?

cc @arielb1, @nagisa

rust-highfive · 2016-05-12T09:06:24Z

r? @nikomatsakis

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

nikomatsakis · 2016-05-12T12:14:56Z

seems...ok to me :) might be nice to be a touch more general (e.g., if there is an exhaustive u8)

well, this commit alone is not enough, in that it invalidates the SwitchInt terminator's invariants. e.g. this comment on targets says:
/// Possible branch sites. The length of this vector should be/// equal to the length of the `values` vector plus 1 -- the/// extra item is the block to branch to if none of the values/// fit. targets:Vec<BasicBlock>,
but here you are not generating the otherwise block. It'd be better to change terminator in that case I imagine?

nagisa · 2016-05-12T13:41:42Z

~~This is already something which would break #33566.~~

~~If you want to land this you’ll have to fix #33567 in tandem.~~

EDIT: oh, this touches SwitchInt only, though, so I guess its fine.

luqmana · 2016-05-12T18:45:25Z

Ok, changed SwitchInt to explicitly separate out the otherwise block and put it behind an Option. Also, made sure it actually generates valid LLVM IR this time and added a test :P

arielb1 · 2016-05-12T22:11:11Z

I think we want to generate an If for booleans.

nikomatsakis · 2016-05-13T00:31:01Z

@arielb1

I think we want to generate an If for booleans.

Or else change If to SwitchInt?

nagisa · 2016-05-13T00:34:47Z

Or else change If to SwitchInt?

While not necessarily incorrect, that would result in a weird branch in trans since LLVM wants a br in boolean case.

luqmana · 2016-05-13T01:45:38Z

We could definitely just take TestKind::SwitchInt to TerminatorKind::If for bool types

nikomatsakis · 2016-05-13T05:21:13Z

@luqmana

We could definitely just take TestKind::SwitchInt to TerminatorKind::If for bool types

I'd be happier with that...

nikomatsakis · 2016-05-13T05:21:36Z

...though I imagine we could make SwitchInt examine the type of the "int" it is matching against?

luqmana · 2016-05-15T22:51:06Z

@nikomatsakis

...though I imagine we could make SwitchInt examine the type of the "int" it is matching against?

As in, SwitchInt during build::matches::test?

nikomatsakis · 2016-05-16T20:35:33Z

@luqmana I think I meant in the trans code. But I'm happy either with using a distinct If terminator or with melding SwitchInt and If.

nikomatsakis · 2016-05-25T20:36:06Z

@luqmana do you plan to update this PR to one of those two strategies? :)

bors · 2016-05-25T22:39:49Z

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

luqmana · 2016-05-28T19:57:07Z

@nikomatsakis Yup sorry, just got occupied with some other work. Anyways, updated the patch to just generate an If terminator for boolean types.

nikomatsakis · 2016-05-31T19:58:25Z

I guess this test should have #[rustc_mir]? And a comment explaining what it's all about?

luqmana · 2016-06-02T00:52:40Z

Added attribute and comment to test.

arielb1 · 2016-06-02T16:36:57Z

@bors r+

bors · 2016-06-02T16:36:57Z

📌 Commit a97f6b3 has been approved by arielb1

bors · 2016-06-02T17:55:43Z

⌛ Testing commit a97f6b3 with merge 12d1653...

@arielb1

MIR: Don't generate 3-armed boolean switch from match. Fixes#33540. Snippet from issue: ```Rust fn foo(x: bool, y: bool) -> u32 { match (x, y) { (false, _) => 0, (_, false) => 1, (true, true) => 2, } } ``` Generated MIR: ``` fn foo(arg0: bool, arg1: bool) -> u32 { let var0: bool; // "x" in scope 1 at 3bbm.rs:17:8: 17:9 let var1: bool; // "y" in scope 1 at 3bbm.rs:17:17: 17:18 let mut tmp0: (bool, bool); let mut tmp1: bool; let mut tmp2: bool; let mut tmp3: (&'static str, &'static str, u32); let mut tmp4: &'static (&'static str, &'static str, u32); bb0: { var0 = arg0; // scope 1 at 3bbm.rs:17:8: 17:9 var1 = arg1; // scope 1 at 3bbm.rs:17:17: 17:18 tmp1 = var0; // scope 5 at 3bbm.rs:18:12: 18:13 tmp2 = var1; // scope 6 at 3bbm.rs:18:15: 18:16 tmp0 = (tmp1, tmp2); // scope 4 at 3bbm.rs:18:11: 18:17 if((tmp0.0: bool)) -> [true: bb4, false: bb1]; // scope 3 at 3bbm.rs:19:10: 19:15 } bb1: { return = const 0u32; // scope 10 at 3bbm.rs:19:23: 19:24 goto -> bb7; // scope 3 at 3bbm.rs:18:5: 22:6 } bb2: { return = const 1u32; // scope 11 at 3bbm.rs:20:23: 20:24 goto -> bb7; // scope 3 at 3bbm.rs:18:5: 22:6 } bb3: { return = const 2u32; // scope 12 at 3bbm.rs:21:25: 21:26 goto -> bb7; // scope 3 at 3bbm.rs:18:5: 22:6 } bb4: { if((tmp0.1: bool)) -> [true: bb5, false: bb2]; // scope 3 at 3bbm.rs:20:13: 20:18 } bb5: { if((tmp0.0: bool)) -> [true: bb3, false: bb6]; // scope 3 at 3bbm.rs:21:10: 21:14 } bb6: { tmp4 = promoted0; // scope 3 at 3bbm.rs:18:5: 22:6 core::panicking::panic(tmp4); // scope 3 at 3bbm.rs:18:5: 22:6 } bb7: { return; // scope 0 at 3bbm.rs:17:1: 23:2 } } ``` Not sure about this approach. I was also thinking maybe just a standalone pass? cc @arielb1, @nagisa

bors · 2016-06-02T20:52:35Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cargotest, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, 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-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

rust-highfive assigned nikomatsakis May 12, 2016

nikomatsakis reviewed May 12, 2016
View reviewed changes

luqmana force-pushed the tri-bool-mir branch from 94faf89 to 0ecd92dCompare May 12, 2016 22:01

luqmana force-pushed the tri-bool-mir branch from 0ecd92d to 48bca85Compare May 28, 2016 19:50

nikomatsakis reviewed May 31, 2016
View reviewed changes

luqmana force-pushed the tri-bool-mir branch from 48bca85 to b596565Compare June 2, 2016 00:52

[MIR] Use If terminator for switches on bools rather than SwitchInt.
a97f6b3

luqmana force-pushed the tri-bool-mir branch from b596565 to a97f6b3Compare June 2, 2016 01:02

bors merged commit a97f6b3 into rust-lang:masterJun 2, 2016

bors mentioned this pull request Jun 2, 2016
[MIR] Make scopes debuginfo-specific (visibility scopes). #33989
Merged

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Uh oh!

rust-highfive commented May 12, 2016

Uh oh!

nikomatsakisMay 12, 2016

Choose a reason for hiding this comment

Uh oh!

nikomatsakisMay 12, 2016

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

luqmana commented May 12, 2016

Uh oh!

arielb1 commented May 12, 2016

Uh oh!

nikomatsakis commented May 13, 2016

Uh oh!

nagisa commented May 13, 2016

Uh oh!

luqmana commented May 13, 2016

Uh oh!

nikomatsakis commented May 13, 2016

Uh oh!

nikomatsakis commented May 13, 2016

Uh oh!

luqmana commented May 15, 2016

Uh oh!

nikomatsakis commented May 16, 2016

Uh oh!

nikomatsakis commented May 25, 2016

Uh oh!

bors commented May 25, 2016

Uh oh!

luqmana commented May 28, 2016

Uh oh!

nikomatsakisMay 31, 2016

Choose a reason for hiding this comment

Uh oh!

luqmana commented Jun 2, 2016

Uh oh!

arielb1 commented Jun 2, 2016

Uh oh!

bors commented Jun 2, 2016

Uh oh!

bors commented Jun 2, 2016

Uh oh!

bors commented Jun 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

luqmana commented May 12, 2016•
edited
Loading

nagisa commented May 12, 2016•
edited
Loading