Skip to content

Commit

Permalink
Auto merge of #45821 - djzin:unreachable-match-arms, r=nikomatsakis
Browse files Browse the repository at this point in the history 
always add an unreachable branch on matches to give more info to llvm

As part of https://github.com/djzin/rustc-optimization I discovered that some simple enum optimizations (src/unary/three_valued_enum.rs and src/unary/four_valued_enum.rs in the repo) are not applied - and the reason for this is that we erase the info that the discriminant of an enum is one of the options by putting the last one in an "otherwise" branch. This patch adds an extra branch so that LLVM can know what the possibilities are for the discriminant, which fixes the three- and four- valued cases.

Note that for whatever reason, this doesn't fix the case of 2 variants (most notably `Option` and `Result` have 2 variants) - a pass re-ordering might fix this or we may wish to add "assume" annotations on discriminants to force it to optimize.
  • Loading branch information
@bors
bors committed Nov 14, 2017
2 parents dbeb5bf + 5b1cc1d commit ff0f5de
Show file tree
Hide file tree
Showing 5 changed files with 103 additions and 31 deletions.
2 changes: 1 addition & 1 deletion src/librustc_mir/build/matches/test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
if let Some(otherwise_block) = otherwise_block {
targets.push(otherwise_block);
} else {
values.pop();
targets.push(self.unreachable_block());
}
debug!("num_enum_variants: {}, tested variants: {:?}, variants: {:?}",
num_enum_variants, values, variants);
Expand Down
21 changes: 20 additions & 1 deletion src/librustc_mir/build/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,8 @@ struct Builder<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
cached_resume_block: Option<BasicBlock>,
/// cached block with the RETURN terminator
cached_return_block: Option<BasicBlock>,
/// cached block with the UNREACHABLE terminator
cached_unreachable_block: Option<BasicBlock>,
}

struct CFG<'tcx> {
Expand Down Expand Up @@ -400,6 +402,11 @@ fn construct_fn<'a, 'gcx, 'tcx, A>(hir: Cx<'a, 'gcx, 'tcx>,
TerminatorKind::Goto { target: return_block });
builder.cfg.terminate(return_block, source_info,
TerminatorKind::Return);
// Attribute any unreachable codepaths to the function's closing brace
if let Some(unreachable_block) = builder.cached_unreachable_block {
builder.cfg.terminate(unreachable_block, source_info,
TerminatorKind::Unreachable);
}
return_block.unit()
}));
assert_eq!(block, builder.return_block());
Expand Down Expand Up @@ -502,7 +509,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
var_indices: NodeMap(),
unit_temp: None,
cached_resume_block: None,
cached_return_block: None
cached_return_block: None,
cached_unreachable_block: None,
};

assert_eq!(builder.cfg.start_new_block(), START_BLOCK);
Expand Down Expand Up @@ -631,6 +639,17 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
}
}
}

fn unreachable_block(&mut self) -> BasicBlock {
match self.cached_unreachable_block {
Some(ub) => ub,
None => {
let ub = self.cfg.start_new_block();
self.cached_unreachable_block = Some(ub);
ub
}
}
}
}

///////////////////////////////////////////////////////////////////////////
Expand Down
44 changes: 44 additions & 0 deletions src/test/codegen/match-optimizes-away.rs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.
//
// no-system-llvm
// compile-flags: -O
#![crate_type="lib"]

pub enum Three { First, Second, Third }
use Three::*;

pub enum Four { First, Second, Third, Fourth }
use Four::*;

#[no_mangle]
pub fn three_valued(x: Three) -> Three {
// CHECK-LABEL: @three_valued
// CHECK-NEXT: {{^.*:$}}
// CHECK-NEXT: ret i8 %0
match x {
First => First,
Second => Second,
Third => Third,
}
}

#[no_mangle]
pub fn four_valued(x: Four) -> Four {
// CHECK-LABEL: @four_valued
// CHECK-NEXT: {{^.*:$}}
// CHECK-NEXT: ret i8 %0
match x {
First => First,
Second => Second,
Third => Third,
Fourth => Fourth,
}
}
9 changes: 6 additions & 3 deletions src/test/codegen/match.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,15 @@ pub enum E {
#[no_mangle]
pub fn exhaustive_match(e: E) {
// CHECK: switch{{.*}}, label %[[OTHERWISE:[a-zA-Z0-9_]+]] [
// CHECK-NEXT: i[[TY:[0-9]+]] [[DISCR:[0-9]+]], label %[[TRUE:[a-zA-Z0-9_]+]]
// CHECK-NEXT: i[[TY:[0-9]+]] [[DISCR:[0-9]+]], label %[[A:[a-zA-Z0-9_]+]]
// CHECK-NEXT: i[[TY:[0-9]+]] [[DISCR:[0-9]+]], label %[[B:[a-zA-Z0-9_]+]]
// CHECK-NEXT: ]
// CHECK: [[TRUE]]:
// CHECK: [[A]]:
// CHECK-NEXT: br label %[[EXIT:[a-zA-Z0-9_]+]]
// CHECK: [[OTHERWISE]]:
// CHECK: [[B]]:
// CHECK-NEXT: br label %[[EXIT:[a-zA-Z0-9_]+]]
// CHECK: [[OTHERWISE]]:
// CHECK-NEXT: unreachable
match e {
E::A => (),
E::B => (),
Expand Down
58 changes: 32 additions & 26 deletions src/test/mir-opt/match_false_edges.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,53 +54,56 @@ fn main() {
// ...
// _2 = std::option::Option<i32>::Some(const 42i32,);
// _5 = discriminant(_2);
// switchInt(_5) -> [0isize: bb5, otherwise: bb3];
// switchInt(_5) -> [0isize: bb5, 1isize: bb3, otherwise: bb7];
// }
// bb1: { // arm1
// StorageLive(_7);
// _7 = _3;
// _1 = (const 1i32, _7);
// StorageDead(_7);
// goto -> bb11;
// goto -> bb12;
// }
// bb2: { // binding3(empty) and arm3
// _1 = (const 3i32, const 3i32);
// goto -> bb11;
// goto -> bb12;
// }
// bb3: {
// falseEdges -> [real: bb7, imaginary: bb4]; //pre_binding1
// falseEdges -> [real: bb8, imaginary: bb4]; //pre_binding1
// }
// bb4: {
// falseEdges -> [real: bb10, imaginary: bb5]; //pre_binding2
// falseEdges -> [real: bb11, imaginary: bb5]; //pre_binding2
// }
// bb5: {
// falseEdges -> [real: bb2, imaginary: bb6]; //pre_binding3
// }
// bb6: {
// unreachable;
// }
// bb7: { // binding1 and guard
// bb7: {
// unreachable;
// }
// bb8: { // binding1 and guard
// StorageLive(_3);
// _3 = ((_2 as Some).0: i32);
// StorageLive(_6);
// _6 = const guard() -> bb8;
// _6 = const guard() -> bb9;
// }
// bb8: { // end of guard
// switchInt(_6) -> [0u8: bb9, otherwise: bb1];
// bb9: { // end of guard
// switchInt(_6) -> [0u8: bb10, otherwise: bb1];
// }
// bb9: { // to pre_binding2
// bb10: { // to pre_binding2
// falseEdges -> [real: bb4, imaginary: bb4];
// }
// bb10: { // bindingNoLandingPads.before.mir2 and arm2
// bb11: { // bindingNoLandingPads.before.mir2 and arm2
// StorageLive(_4);
// _4 = ((_2 as Some).0: i32);
// StorageLive(_8);
// _8 = _4;
// _1 = (const 2i32, _8);
// StorageDead(_8);
// goto -> bb11;
// goto -> bb12;
// }
// bb11: {
// bb12: {
// ...
// return;
// }
Expand All @@ -111,53 +114,56 @@ fn main() {
// ...
// _2 = std::option::Option<i32>::Some(const 42i32,);
// _5 = discriminant(_2);
// switchInt(_5) -> [0isize: bb4, otherwise: bb3];
// switchInt(_5) -> [0isize: bb4, 1isize: bb3, otherwise: bb7];
// }
// bb1: { // arm1
// StorageLive(_7);
// _7 = _3;
// _1 = (const 1i32, _7);
// StorageDead(_7);
// goto -> bb11;
// goto -> bb12;
// }
// bb2: { // binding3(empty) and arm3
// _1 = (const 3i32, const 3i32);
// goto -> bb11;
// goto -> bb12;
// }
// bb3: {
// falseEdges -> [real: bb7, imaginary: bb4]; //pre_binding1
// falseEdges -> [real: bb8, imaginary: bb4]; //pre_binding1
// }
// bb4: {
// falseEdges -> [real: bb2, imaginary: bb5]; //pre_binding2
// }
// bb5: {
// falseEdges -> [real: bb10, imaginary: bb6]; //pre_binding3
// falseEdges -> [real: bb11, imaginary: bb6]; //pre_binding3
// }
// bb6: {
// unreachable;
// }
// bb7: { // binding1 and guard
// bb7: {
// unreachable;
// }
// bb8: { // binding1 and guard
// StorageLive(_3);
// _3 = ((_2 as Some).0: i32);
// StorageLive(_6);
// _6 = const guard() -> bb8;
// _6 = const guard() -> bb9;
// }
// bb8: { // end of guard
// switchInt(_6) -> [0u8: bb9, otherwise: bb1];
// bb9: { // end of guard
// switchInt(_6) -> [0u8: bb10, otherwise: bb1];
// }
// bb9: { // to pre_binding2
// bb10: { // to pre_binding2
// falseEdges -> [real: bb5, imaginary: bb4];
// }
// bb10: { // binding2 and arm2
// bb11: { // binding2 and arm2
// StorageLive(_4);
// _4 = ((_2 as Some).0: i32);
// StorageLive(_8);
// _8 = _4;
// _1 = (const 2i32, _8);
// StorageDead(_8);
// goto -> bb11;
// goto -> bb12;
// }
// bb11: {
// bb12: {
// ...
// return;
// }
Expand Down

0 comments on commit ff0f5de

Please sign in to comment.