From 368ca131fa17274227703f76def592899c477c59 Mon Sep 17 00:00:00 2001 From: Jiri Bobek Date: Fri, 19 Jan 2024 21:44:44 +0100 Subject: [PATCH 1/8] Initial implementation of #[cold] attribute on match arms --- compiler/rustc_codegen_llvm/src/builder.rs | 30 ++++++++ compiler/rustc_codegen_ssa/src/mir/block.rs | 15 +++- .../rustc_codegen_ssa/src/traits/builder.rs | 7 ++ .../src/stable_hasher.rs | 11 +++ compiler/rustc_middle/src/mir/syntax.rs | 7 ++ compiler/rustc_middle/src/mir/terminator.rs | 52 ++++++++++++- compiler/rustc_middle/src/thir.rs | 1 + compiler/rustc_mir_build/Cargo.toml | 1 + .../rustc_mir_build/src/build/expr/into.rs | 4 +- .../rustc_mir_build/src/build/matches/mod.rs | 77 ++++++++++++++++--- .../src/build/matches/simplify.rs | 2 +- .../rustc_mir_build/src/build/matches/test.rs | 19 ++++- compiler/rustc_mir_build/src/thir/cx/expr.rs | 2 + compiler/rustc_mir_build/src/thir/print.rs | 3 +- 14 files changed, 210 insertions(+), 21 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/builder.rs b/compiler/rustc_codegen_llvm/src/builder.rs index 7ed27b33dceaa..470326272d37c 100644 --- a/compiler/rustc_codegen_llvm/src/builder.rs +++ b/compiler/rustc_codegen_llvm/src/builder.rs @@ -197,6 +197,36 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { } } + fn cond_br_with_cold_br( + &mut self, + cond: &'ll Value, + then_llbb: Self::BasicBlock, + else_llbb: Self::BasicBlock, + cold_br: Option, + ) { + // emit the branch instruction + let n = unsafe { + llvm::LLVMBuildCondBr(self.llbuilder, cond, then_llbb, else_llbb) + }; + + // if one of the branches is cold, emit metadata with branch weights + if let Some(cold_br) = cold_br { + unsafe { + let s = "branch_weights"; + let v = [ + llvm::LLVMMDStringInContext(self.cx.llcx, s.as_ptr() as *const c_char, s.len() as c_uint), + self.cx.const_u32(if cold_br { 1 } else { 2000 }), // 'then' branch weight + self.cx.const_u32(if cold_br { 2000 } else { 1 }), // 'else' branch weight + ]; + llvm::LLVMSetMetadata( + n, + llvm::MD_prof as c_uint, + llvm::LLVMMDNodeInContext(self.cx.llcx, v.as_ptr(), v.len() as c_uint), + ); + } + } + } + fn switch( &mut self, v: &'ll Value, diff --git a/compiler/rustc_codegen_ssa/src/mir/block.rs b/compiler/rustc_codegen_ssa/src/mir/block.rs index a1662f25e1496..d9f8114810496 100644 --- a/compiler/rustc_codegen_ssa/src/mir/block.rs +++ b/compiler/rustc_codegen_ssa/src/mir/block.rs @@ -327,18 +327,27 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { let (test_value, target) = target_iter.next().unwrap(); let lltrue = helper.llbb_with_cleanup(self, target); let llfalse = helper.llbb_with_cleanup(self, targets.otherwise()); + let cold_br = targets.cold_target().and_then(|t| { + if t == 0 { Some(true) } else { Some(false) } + }); + if switch_ty == bx.tcx().types.bool { // Don't generate trivial icmps when switching on bool. match test_value { - 0 => bx.cond_br(discr.immediate(), llfalse, lltrue), - 1 => bx.cond_br(discr.immediate(), lltrue, llfalse), + 0 => { + let cold_br = cold_br.and_then(|t| Some(!t)); + bx.cond_br_with_cold_br(discr.immediate(), llfalse, lltrue, cold_br); + }, + 1 => { + bx.cond_br_with_cold_br(discr.immediate(), lltrue, llfalse, cold_br) + }, _ => bug!(), } } else { let switch_llty = bx.immediate_backend_type(bx.layout_of(switch_ty)); let llval = bx.const_uint_big(switch_llty, test_value); let cmp = bx.icmp(IntPredicate::IntEQ, discr.immediate(), llval); - bx.cond_br(cmp, lltrue, llfalse); + bx.cond_br_with_cold_br(cmp, lltrue, llfalse, cold_br); } } else if self.cx.sess().opts.optimize == OptLevel::No && target_iter.len() == 2 diff --git a/compiler/rustc_codegen_ssa/src/traits/builder.rs b/compiler/rustc_codegen_ssa/src/traits/builder.rs index 1c5c78e6ca200..9e8ccdd023452 100644 --- a/compiler/rustc_codegen_ssa/src/traits/builder.rs +++ b/compiler/rustc_codegen_ssa/src/traits/builder.rs @@ -64,6 +64,13 @@ pub trait BuilderMethods<'a, 'tcx>: then_llbb: Self::BasicBlock, else_llbb: Self::BasicBlock, ); + fn cond_br_with_cold_br( + &mut self, + cond: Self::Value, + then_llbb: Self::BasicBlock, + else_llbb: Self::BasicBlock, + cold_br: Option, + ); fn switch( &mut self, v: Self::Value, diff --git a/compiler/rustc_data_structures/src/stable_hasher.rs b/compiler/rustc_data_structures/src/stable_hasher.rs index 6d75b0fb8a0ca..8b31ebf6cd522 100644 --- a/compiler/rustc_data_structures/src/stable_hasher.rs +++ b/compiler/rustc_data_structures/src/stable_hasher.rs @@ -2,6 +2,7 @@ use crate::sip128::SipHasher128; use rustc_index::bit_set::{self, BitSet}; use rustc_index::{Idx, IndexSlice, IndexVec}; use smallvec::SmallVec; +use thin_vec::ThinVec; use std::fmt; use std::hash::{BuildHasher, Hash, Hasher}; use std::marker::PhantomData; @@ -461,6 +462,16 @@ where } } +impl HashStable for ThinVec +where + T: HashStable, +{ + #[inline] + fn hash_stable(&self, ctx: &mut CTX, hasher: &mut StableHasher) { + self[..].hash_stable(ctx, hasher); + } +} + impl, CTX> HashStable for Box { #[inline] fn hash_stable(&self, ctx: &mut CTX, hasher: &mut StableHasher) { diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs index 462076d750f27..a8170ebfce0e8 100644 --- a/compiler/rustc_middle/src/mir/syntax.rs +++ b/compiler/rustc_middle/src/mir/syntax.rs @@ -24,6 +24,7 @@ use rustc_span::symbol::Symbol; use rustc_span::Span; use rustc_target::asm::InlineAsmRegOrRegClass; use smallvec::SmallVec; +use thin_vec::ThinVec; /// Represents the "flavors" of MIR. /// @@ -841,6 +842,12 @@ pub struct SwitchTargets { // However we’ve decided to keep this as-is until we figure a case // where some other approach seems to be strictly better than other. pub(super) targets: SmallVec<[BasicBlock; 2]>, + + // Targets that are marked 'cold', if any. + // This vector contains indices into `targets`. + // It can also contain 'targets.len()' to indicate that the otherwise + // branch is cold. + pub(super) cold_targets: ThinVec, } /// Action to be taken when a stack unwind happens. diff --git a/compiler/rustc_middle/src/mir/terminator.rs b/compiler/rustc_middle/src/mir/terminator.rs index 7be6deb614193..7c2f5f58287ad 100644 --- a/compiler/rustc_middle/src/mir/terminator.rs +++ b/compiler/rustc_middle/src/mir/terminator.rs @@ -5,6 +5,7 @@ use smallvec::SmallVec; use super::TerminatorKind; use rustc_macros::HashStable; use std::slice; +use thin_vec::{thin_vec, ThinVec}; use super::*; @@ -16,13 +17,36 @@ impl SwitchTargets { pub fn new(targets: impl Iterator, otherwise: BasicBlock) -> Self { let (values, mut targets): (SmallVec<_>, SmallVec<_>) = targets.unzip(); targets.push(otherwise); - Self { values, targets } + Self { values, targets, cold_targets: ThinVec::new() } } /// Builds a switch targets definition that jumps to `then` if the tested value equals `value`, /// and to `else_` if not. pub fn static_if(value: u128, then: BasicBlock, else_: BasicBlock) -> Self { - Self { values: smallvec![value], targets: smallvec![then, else_] } + Self { + values: smallvec![value], + targets: smallvec![then, else_], + cold_targets: ThinVec::new(), + } + } + + /// Builds a switch targets definition that jumps to `then` if the tested value equals `value`, + /// and to `else_` if not. + /// If cold_br is some bool value, the given outcome is considered cold (i.e., unlikely). + pub fn static_if_with_cold_br( + value: u128, + then: BasicBlock, + else_: BasicBlock, + cold_br: Option, + ) -> Self { + Self { + values: smallvec![value], + targets: smallvec![then, else_], + cold_targets: match cold_br { + Some(br) => thin_vec![if br { 0 } else { 1 }], + None => ThinVec::new(), + }, + } } /// Inverse of `SwitchTargets::static_if`. @@ -36,6 +60,14 @@ impl SwitchTargets { } } + pub fn cold_target(&self) -> Option { + if self.cold_targets.len() == 1 { + Some(self.cold_targets[0]) + } else { + None + } + } + /// Returns the fallback target that is jumped to when none of the values match the operand. pub fn otherwise(&self) -> BasicBlock { *self.targets.last().unwrap() @@ -352,6 +384,22 @@ impl<'tcx> TerminatorKind<'tcx> { TerminatorKind::SwitchInt { discr: cond, targets: SwitchTargets::static_if(0, f, t) } } + pub fn if_with_cold_br( + cond: Operand<'tcx>, + t: BasicBlock, + f: BasicBlock, + cold_branch: Option, + ) -> TerminatorKind<'tcx> { + TerminatorKind::SwitchInt { + discr: cond, + targets: SwitchTargets::static_if_with_cold_br( + 0, f, t, + // we compare to zero, so have to invert the branch + cold_branch.and_then(|b| Some(!b)) + ), + } + } + pub fn successors(&self) -> Successors<'_> { use self::TerminatorKind::*; match *self { diff --git a/compiler/rustc_middle/src/thir.rs b/compiler/rustc_middle/src/thir.rs index b6759d3521023..25dfa3c245001 100644 --- a/compiler/rustc_middle/src/thir.rs +++ b/compiler/rustc_middle/src/thir.rs @@ -524,6 +524,7 @@ pub struct Arm<'tcx> { pub lint_level: LintLevel, pub scope: region::Scope, pub span: Span, + pub is_cold: bool, } /// A `match` guard. diff --git a/compiler/rustc_mir_build/Cargo.toml b/compiler/rustc_mir_build/Cargo.toml index 6d681dc295efb..ba41175a2c66a 100644 --- a/compiler/rustc_mir_build/Cargo.toml +++ b/compiler/rustc_mir_build/Cargo.toml @@ -23,5 +23,6 @@ rustc_span = { path = "../rustc_span" } rustc_target = { path = "../rustc_target" } rustc_trait_selection = { path = "../rustc_trait_selection" } smallvec = { version = "1.8.1", features = ["union", "may_dangle"] } +thin-vec = "0.2.12" tracing = "0.1" # tidy-alphabetical-end diff --git a/compiler/rustc_mir_build/src/build/expr/into.rs b/compiler/rustc_mir_build/src/build/expr/into.rs index f50945a4de05d..9b1691ef6cb36 100644 --- a/compiler/rustc_mir_build/src/build/expr/into.rs +++ b/compiler/rustc_mir_build/src/build/expr/into.rs @@ -82,7 +82,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { cond, Some(condition_scope), condition_scope, - source_info + source_info, + None, )); this.expr_into_dest(destination, then_blk, then) @@ -173,6 +174,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { Some(condition_scope), condition_scope, source_info, + None, ) }); let (short_circuit, continuation, constant) = match op { diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 7f29e3308f4f5..0cfe13acbf397 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -23,6 +23,7 @@ use rustc_span::symbol::Symbol; use rustc_span::{BytePos, Pos, Span}; use rustc_target::abi::VariantIdx; use smallvec::{smallvec, SmallVec}; +use thin_vec::ThinVec; // helper functions, broken out by category: mod simplify; @@ -40,6 +41,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { temp_scope_override: Option, break_scope: region::Scope, variable_source_info: SourceInfo, + cold_branch: Option, ) -> BlockAnd<()> { let this = self; let expr = &this.thir[expr_id]; @@ -53,6 +55,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { temp_scope_override, break_scope, variable_source_info, + match cold_branch { Some(false) => Some(false), _ => None } )); let rhs_then_block = unpack!(this.then_else_break( @@ -61,6 +64,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { temp_scope_override, break_scope, variable_source_info, + cold_branch, )); rhs_then_block.unit() @@ -75,6 +79,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { temp_scope_override, local_scope, variable_source_info, + match cold_branch { Some(true) => Some(true), _ => None } ) }); let rhs_success_block = unpack!(this.then_else_break( @@ -83,6 +88,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { temp_scope_override, break_scope, variable_source_info, + cold_branch, )); this.cfg.goto(lhs_success_block, variable_source_info, rhs_success_block); rhs_success_block.unit() @@ -102,6 +108,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { temp_scope_override, local_scope, variable_source_info, + cold_branch.and_then(|b| Some(!b)), ) }); this.break_for_else(success_block, break_scope, variable_source_info); @@ -116,6 +123,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { temp_scope_override, break_scope, variable_source_info, + cold_branch, ) }) } @@ -125,6 +133,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { temp_scope_override, break_scope, variable_source_info, + cold_branch, ), ExprKind::Let { expr, ref pat } => this.lower_let_expr( block, @@ -144,7 +153,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let then_block = this.cfg.start_new_block(); let else_block = this.cfg.start_new_block(); - let term = TerminatorKind::if_(operand, then_block, else_block); + let term = TerminatorKind::if_with_cold_br( + operand, + then_block, + else_block, + cold_branch + ); let source_info = this.source_info(expr_span); this.cfg.terminate(block, source_info, term); @@ -278,8 +292,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { .map(|arm| { let arm = &self.thir[arm]; let arm_has_guard = arm.guard.is_some(); + let arm_is_cold = arm.is_cold; let arm_candidate = - Candidate::new(scrutinee.clone(), &arm.pattern, arm_has_guard, self); + Candidate::new(scrutinee.clone(), &arm.pattern, arm_has_guard, arm_is_cold, self); (arm, arm_candidate) }) .collect() @@ -638,7 +653,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { initializer: PlaceBuilder<'tcx>, set_match_place: bool, ) -> BlockAnd<()> { - let mut candidate = Candidate::new(initializer.clone(), irrefutable_pat, false, self); + let mut candidate = Candidate::new(initializer.clone(), irrefutable_pat, false, false, self); let fake_borrow_temps = self.lower_match_tree( block, irrefutable_pat.span, @@ -909,6 +924,9 @@ struct Candidate<'pat, 'tcx> { /// Whether this `Candidate` has a guard. has_guard: bool, + /// Whether this 'Candidate' comes from a cold arm. + is_cold: bool, + /// All of these must be satisfied... match_pairs: SmallVec<[MatchPair<'pat, 'tcx>; 1]>, @@ -935,11 +953,13 @@ impl<'tcx, 'pat> Candidate<'pat, 'tcx> { place: PlaceBuilder<'tcx>, pattern: &'pat Pat<'tcx>, has_guard: bool, + is_cold: bool, cx: &Builder<'_, 'tcx>, ) -> Self { Candidate { span: pattern.span, has_guard, + is_cold, match_pairs: smallvec![MatchPair::new(place, pattern, cx)], bindings: Vec::new(), ascriptions: Vec::new(), @@ -1459,7 +1479,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { debug!("candidate={:#?}\npats={:#?}", candidate, pats); let mut or_candidates: Vec<_> = pats .iter() - .map(|pat| Candidate::new(place.clone(), pat, candidate.has_guard, self)) + .map(|pat| Candidate::new(place.clone(), pat, candidate.has_guard, candidate.is_cold, self)) .collect(); let mut or_candidate_refs: Vec<_> = or_candidates.iter_mut().collect(); let otherwise = if candidate.otherwise_block.is_some() { @@ -1707,6 +1727,36 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { debug!("tested_candidates: {}", total_candidate_count - candidates.len()); debug!("untested_candidates: {}", candidates.len()); + // Find cold targets. + let mut cold_targets: ThinVec = ThinVec::new(); + + let is_otherwise_target_cold = + candidates.len() > 0 && + candidates.iter().all(|c| c.is_cold);// && c.match_pairs.is_empty()); + if is_otherwise_target_cold { + cold_targets.push(target_candidates.len()); + } + + for (target, candidates) in target_candidates.iter().enumerate() { + let cold = + // If all candidates for this target are cold, then the target is cold. + ( + candidates.len() > 0 && + candidates.iter().all(|c| c.is_cold) //&& c.match_pairs.is_empty()) + ) || + // This should really only happen for 'bool', because the switch for bool + // always has two targets. One of the targets may be without candidates, + // and therefore equivalent to the 'otherwise' target. + ( + candidates.len() == 0 && + is_otherwise_target_cold + ); + + if cold { + cold_targets.push(target); + } + } + // The block that we should branch to if none of the // `target_candidates` match. This is either the block where we // start matching the untested candidates if there are any, @@ -1751,7 +1801,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { ); } - self.perform_test(span, scrutinee_span, block, &match_place, &test, target_blocks); + self.perform_test( + span, + scrutinee_span, + block, + &match_place, + &test, + target_blocks, + cold_targets + ); } /// Determine the fake borrows that are needed from a set of places that @@ -1849,9 +1907,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let expr_span = self.thir[expr_id].span; let expr_place_builder = unpack!(block = self.lower_scrutinee(block, expr_id, expr_span)); let wildcard = Pat::wildcard_from_ty(pat.ty); - let mut guard_candidate = Candidate::new(expr_place_builder.clone(), pat, false, self); + let mut guard_candidate = Candidate::new(expr_place_builder.clone(), pat, false, false, self); let mut otherwise_candidate = - Candidate::new(expr_place_builder.clone(), &wildcard, false, self); + Candidate::new(expr_place_builder.clone(), &wildcard, false, false, self); let fake_borrow_temps = self.lower_match_tree( block, pat.span, @@ -2043,6 +2101,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { None, match_scope, this.source_info(arm.span), + if candidate.is_cold { Some(false) } else { None }, ) } Guard::IfLet(ref pat, s) => { @@ -2350,8 +2409,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let (matching, failure) = self.in_if_then_scope(*let_else_scope, else_block_span, |this| { let scrutinee = unpack!(block = this.lower_scrutinee(block, init_id, initializer_span)); let pat = Pat { ty: pattern.ty, span: else_block_span, kind: PatKind::Wild }; - let mut wildcard = Candidate::new(scrutinee.clone(), &pat, false, this); - let mut candidate = Candidate::new(scrutinee.clone(), pattern, false, this); + let mut wildcard = Candidate::new(scrutinee.clone(), &pat, false, false, this); + let mut candidate = Candidate::new(scrutinee.clone(), pattern, false, false, this); let fake_borrow_temps = this.lower_match_tree( block, initializer_span, diff --git a/compiler/rustc_mir_build/src/build/matches/simplify.rs b/compiler/rustc_mir_build/src/build/matches/simplify.rs index a7f6f4873e383..2bca55520d89c 100644 --- a/compiler/rustc_mir_build/src/build/matches/simplify.rs +++ b/compiler/rustc_mir_build/src/build/matches/simplify.rs @@ -127,7 +127,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { ) -> Vec> { pats.iter() .map(|box pat| { - let mut candidate = Candidate::new(place.clone(), pat, candidate.has_guard, self); + let mut candidate = Candidate::new(place.clone(), pat, candidate.has_guard, candidate.is_cold, self); self.simplify_candidate(&mut candidate); candidate }) diff --git a/compiler/rustc_mir_build/src/build/matches/test.rs b/compiler/rustc_mir_build/src/build/matches/test.rs index 53e5d70f946eb..21ebbd3b4fe12 100644 --- a/compiler/rustc_mir_build/src/build/matches/test.rs +++ b/compiler/rustc_mir_build/src/build/matches/test.rs @@ -22,6 +22,7 @@ use rustc_span::Span; use rustc_target::abi::VariantIdx; use std::cmp::Ordering; +use thin_vec::ThinVec; impl<'a, 'tcx> Builder<'a, 'tcx> { /// Identifies what test is needed to decide if `match_pair` is applicable. @@ -156,6 +157,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { place_builder: &PlaceBuilder<'tcx>, test: &Test<'tcx>, target_blocks: Vec, + cold_targets: ThinVec, ) { let place = place_builder.to_place(self); let place_ty = place.ty(&self.local_decls, self.tcx); @@ -211,15 +213,24 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { TestKind::SwitchInt { switch_ty, ref options } => { let terminator = if *switch_ty.kind() == ty::Bool { assert!(!options.is_empty() && options.len() <= 2); + let mut cold_br = [false; 2]; + let cold_br = if cold_targets.is_empty() { + None + } else { + for i in cold_targets.iter() { + if *i < 2 { cold_br[*i] = true; } + } + if cold_br[0] != cold_br[1] { Some(cold_br[0]) } else { None } + }; let [first_bb, second_bb] = *target_blocks else { bug!("`TestKind::SwitchInt` on `bool` should have two targets") }; - let (true_bb, false_bb) = match options[0] { - 1 => (first_bb, second_bb), - 0 => (second_bb, first_bb), + let (true_bb, false_bb, cold_br) = match options[0] { + 1 => (first_bb, second_bb, cold_br), + 0 => (second_bb, first_bb, cold_br.and_then(|x| Some(!x))), v => span_bug!(test.span, "expected boolean value but got {:?}", v), }; - TerminatorKind::if_(Operand::Copy(place), true_bb, false_bb) + TerminatorKind::if_with_cold_br(Operand::Copy(place), true_bb, false_bb, cold_br) } else { // The switch may be inexhaustive so we have a catch all block debug_assert_eq!(options.len() + 1, target_blocks.len()); diff --git a/compiler/rustc_mir_build/src/thir/cx/expr.rs b/compiler/rustc_mir_build/src/thir/cx/expr.rs index 8ec70c58c4618..a15c515668e55 100644 --- a/compiler/rustc_mir_build/src/thir/cx/expr.rs +++ b/compiler/rustc_mir_build/src/thir/cx/expr.rs @@ -854,6 +854,7 @@ impl<'tcx> Cx<'tcx> { } fn convert_arm(&mut self, arm: &'tcx hir::Arm<'tcx>) -> ArmId { + let attrs = self.tcx.hir().attrs(arm.hir_id); let arm = Arm { pattern: self.pattern_from_hir(arm.pat), guard: arm.guard.as_ref().map(|g| match g { @@ -866,6 +867,7 @@ impl<'tcx> Cx<'tcx> { lint_level: LintLevel::Explicit(arm.hir_id), scope: region::Scope { id: arm.hir_id.local_id, data: region::ScopeData::Node }, span: arm.span, + is_cold: attrs.iter().any(|a| a.name_or_empty() == sym::cold), }; self.thir.arms.push(arm) } diff --git a/compiler/rustc_mir_build/src/thir/print.rs b/compiler/rustc_mir_build/src/thir/print.rs index 28be313990594..7d80a414efc5c 100644 --- a/compiler/rustc_mir_build/src/thir/print.rs +++ b/compiler/rustc_mir_build/src/thir/print.rs @@ -588,7 +588,7 @@ impl<'a, 'tcx> ThirPrinter<'a, 'tcx> { print_indented!(self, "Arm {", depth_lvl); let arm = &self.thir.arms[arm_id]; - let Arm { pattern, guard, body, lint_level, scope, span } = arm; + let Arm { pattern, guard, body, lint_level, scope, span, is_cold } = arm; print_indented!(self, "pattern: ", depth_lvl + 1); self.print_pat(pattern, depth_lvl + 2); @@ -605,6 +605,7 @@ impl<'a, 'tcx> ThirPrinter<'a, 'tcx> { print_indented!(self, format!("lint_level: {:?}", lint_level), depth_lvl + 1); print_indented!(self, format!("scope: {:?}", scope), depth_lvl + 1); print_indented!(self, format!("span: {:?}", span), depth_lvl + 1); + print_indented!(self, format!("is_cold: {:?}", is_cold), depth_lvl + 1); print_indented!(self, "}", depth_lvl); } From 04e1f916e5be4020c78ce81b2d97e12e9ec3a39f Mon Sep 17 00:00:00 2001 From: Jiri Bobek Date: Fri, 19 Jan 2024 21:52:44 +0100 Subject: [PATCH 2/8] Cargo.lock: forgotten file with dependencies --- Cargo.lock | 1 + 1 file changed, 1 insertion(+) diff --git a/Cargo.lock b/Cargo.lock index b8192e333fe91..2dee168881d96 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4206,6 +4206,7 @@ dependencies = [ "rustc_target", "rustc_trait_selection", "smallvec", + "thin-vec", "tracing", ] From 71b694c17a2cd6df7c7e87470671b6accd4bbd2d Mon Sep 17 00:00:00 2001 From: Jiri Bobek Date: Sat, 20 Jan 2024 11:10:31 +0100 Subject: [PATCH 3/8] formatting changes to pass 'test tidy' --- compiler/rustc_codegen_llvm/src/builder.rs | 10 +++-- compiler/rustc_codegen_ssa/src/mir/block.rs | 11 ++--- .../src/stable_hasher.rs | 2 +- compiler/rustc_middle/src/mir/terminator.rs | 15 +++---- .../rustc_mir_build/src/build/matches/mod.rs | 45 +++++++++++-------- .../src/build/matches/simplify.rs | 8 +++- .../rustc_mir_build/src/build/matches/test.rs | 11 ++++- 7 files changed, 61 insertions(+), 41 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/builder.rs b/compiler/rustc_codegen_llvm/src/builder.rs index 470326272d37c..a2aead1228053 100644 --- a/compiler/rustc_codegen_llvm/src/builder.rs +++ b/compiler/rustc_codegen_llvm/src/builder.rs @@ -205,16 +205,18 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { cold_br: Option, ) { // emit the branch instruction - let n = unsafe { - llvm::LLVMBuildCondBr(self.llbuilder, cond, then_llbb, else_llbb) - }; + let n = unsafe { llvm::LLVMBuildCondBr(self.llbuilder, cond, then_llbb, else_llbb) }; // if one of the branches is cold, emit metadata with branch weights if let Some(cold_br) = cold_br { unsafe { let s = "branch_weights"; let v = [ - llvm::LLVMMDStringInContext(self.cx.llcx, s.as_ptr() as *const c_char, s.len() as c_uint), + llvm::LLVMMDStringInContext( + self.cx.llcx, + s.as_ptr() as *const c_char, + s.len() as c_uint, + ), self.cx.const_u32(if cold_br { 1 } else { 2000 }), // 'then' branch weight self.cx.const_u32(if cold_br { 2000 } else { 1 }), // 'else' branch weight ]; diff --git a/compiler/rustc_codegen_ssa/src/mir/block.rs b/compiler/rustc_codegen_ssa/src/mir/block.rs index e8f3ca0bbecd3..1193bd7b2736d 100644 --- a/compiler/rustc_codegen_ssa/src/mir/block.rs +++ b/compiler/rustc_codegen_ssa/src/mir/block.rs @@ -327,9 +327,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { let (test_value, target) = target_iter.next().unwrap(); let lltrue = helper.llbb_with_cleanup(self, target); let llfalse = helper.llbb_with_cleanup(self, targets.otherwise()); - let cold_br = targets.cold_target().and_then(|t| { - if t == 0 { Some(true) } else { Some(false) } - }); + let cold_br = + targets.cold_target().and_then(|t| if t == 0 { Some(true) } else { Some(false) }); if switch_ty == bx.tcx().types.bool { // Don't generate trivial icmps when switching on bool. @@ -337,10 +336,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { 0 => { let cold_br = cold_br.and_then(|t| Some(!t)); bx.cond_br_with_cold_br(discr.immediate(), llfalse, lltrue, cold_br); - }, - 1 => { - bx.cond_br_with_cold_br(discr.immediate(), lltrue, llfalse, cold_br) - }, + } + 1 => bx.cond_br_with_cold_br(discr.immediate(), lltrue, llfalse, cold_br), _ => bug!(), } } else { diff --git a/compiler/rustc_data_structures/src/stable_hasher.rs b/compiler/rustc_data_structures/src/stable_hasher.rs index 884fcf0f5b478..328c8d97095c4 100644 --- a/compiler/rustc_data_structures/src/stable_hasher.rs +++ b/compiler/rustc_data_structures/src/stable_hasher.rs @@ -2,11 +2,11 @@ use crate::sip128::SipHasher128; use rustc_index::bit_set::{self, BitSet}; use rustc_index::{Idx, IndexSlice, IndexVec}; use smallvec::SmallVec; -use thin_vec::ThinVec; use std::fmt; use std::hash::{BuildHasher, Hash, Hasher}; use std::marker::PhantomData; use std::mem; +use thin_vec::ThinVec; #[cfg(test)] mod tests; diff --git a/compiler/rustc_middle/src/mir/terminator.rs b/compiler/rustc_middle/src/mir/terminator.rs index ab2b67bde891f..4ce52b4afe583 100644 --- a/compiler/rustc_middle/src/mir/terminator.rs +++ b/compiler/rustc_middle/src/mir/terminator.rs @@ -61,12 +61,9 @@ impl SwitchTargets { } } + // If this switch has exactly one target, returns it. pub fn cold_target(&self) -> Option { - if self.cold_targets.len() == 1 { - Some(self.cold_targets[0]) - } else { - None - } + if self.cold_targets.len() == 1 { Some(self.cold_targets[0]) } else { None } } /// Returns the fallback target that is jumped to when none of the values match the operand. @@ -402,13 +399,15 @@ impl<'tcx> TerminatorKind<'tcx> { t: BasicBlock, f: BasicBlock, cold_branch: Option, - ) -> TerminatorKind<'tcx> { + ) -> TerminatorKind<'tcx> { TerminatorKind::SwitchInt { discr: cond, targets: SwitchTargets::static_if_with_cold_br( - 0, f, t, + 0, + f, + t, // we compare to zero, so have to invert the branch - cold_branch.and_then(|b| Some(!b)) + cold_branch.and_then(|b| Some(!b)), ), } } diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 57b41ffd6e899..0e39d64a6a3c2 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -63,7 +63,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { break_scope, variable_source_info, declare_bindings, - match cold_branch { Some(false) => Some(false), _ => None }, + match cold_branch { + Some(false) => Some(false), + _ => None, + }, )); let rhs_then_block = unpack!(this.then_else_break( @@ -89,7 +92,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { local_scope, variable_source_info, true, - match cold_branch { Some(true) => Some(true), _ => None }, + match cold_branch { + Some(true) => Some(true), + _ => None, + }, ) }); let rhs_success_block = unpack!(this.then_else_break( @@ -167,12 +173,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let then_block = this.cfg.start_new_block(); let else_block = this.cfg.start_new_block(); - let term = TerminatorKind::if_with_cold_br( - operand, - then_block, - else_block, - cold_branch - ); + let term = + TerminatorKind::if_with_cold_br(operand, then_block, else_block, cold_branch); let source_info = this.source_info(expr_span); this.cfg.terminate(block, source_info, term); @@ -307,8 +309,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let arm = &self.thir[arm]; let arm_has_guard = arm.guard.is_some(); let arm_is_cold = arm.is_cold; - let arm_candidate = - Candidate::new(scrutinee.clone(), &arm.pattern, arm_has_guard, arm_is_cold, self); + let arm_candidate = Candidate::new( + scrutinee.clone(), + &arm.pattern, + arm_has_guard, + arm_is_cold, + self, + ); (arm, arm_candidate) }) .collect() @@ -667,7 +674,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { initializer: PlaceBuilder<'tcx>, set_match_place: bool, ) -> BlockAnd<()> { - let mut candidate = Candidate::new(initializer.clone(), irrefutable_pat, false, false, self); + let mut candidate = + Candidate::new(initializer.clone(), irrefutable_pat, false, false, self); let fake_borrow_temps = self.lower_match_tree( block, irrefutable_pat.span, @@ -1520,7 +1528,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { debug!("candidate={:#?}\npats={:#?}", candidate, pats); let mut or_candidates: Vec<_> = pats .iter() - .map(|pat| Candidate::new(place.clone(), pat, candidate.has_guard, candidate.is_cold, self)) + .map(|pat| { + Candidate::new(place.clone(), pat, candidate.has_guard, candidate.is_cold, self) + }) .collect(); let mut or_candidate_refs: Vec<_> = or_candidates.iter_mut().collect(); let otherwise = if candidate.otherwise_block.is_some() { @@ -1771,10 +1781,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // Find cold targets. let mut cold_targets: ThinVec = ThinVec::new(); - let is_otherwise_target_cold = - candidates.len() > 0 && - candidates.iter().all(|c| c.is_cold);// && c.match_pairs.is_empty()); - if is_otherwise_target_cold { + let is_otherwise_target_cold = candidates.len() > 0 && candidates.iter().all(|c| c.is_cold); + if is_otherwise_target_cold { cold_targets.push(target_candidates.len()); } @@ -1849,7 +1857,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { &match_place, &test, target_blocks, - cold_targets + cold_targets, ); } @@ -1948,7 +1956,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let expr_span = self.thir[expr_id].span; let expr_place_builder = unpack!(block = self.lower_scrutinee(block, expr_id, expr_span)); let wildcard = Pat::wildcard_from_ty(pat.ty); - let mut guard_candidate = Candidate::new(expr_place_builder.clone(), pat, false, false, self); + let mut guard_candidate = + Candidate::new(expr_place_builder.clone(), pat, false, false, self); let mut otherwise_candidate = Candidate::new(expr_place_builder.clone(), &wildcard, false, false, self); let fake_borrow_temps = self.lower_match_tree( diff --git a/compiler/rustc_mir_build/src/build/matches/simplify.rs b/compiler/rustc_mir_build/src/build/matches/simplify.rs index 2bca55520d89c..198c8cff20c42 100644 --- a/compiler/rustc_mir_build/src/build/matches/simplify.rs +++ b/compiler/rustc_mir_build/src/build/matches/simplify.rs @@ -127,7 +127,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { ) -> Vec> { pats.iter() .map(|box pat| { - let mut candidate = Candidate::new(place.clone(), pat, candidate.has_guard, candidate.is_cold, self); + let mut candidate = Candidate::new( + place.clone(), + pat, + candidate.has_guard, + candidate.is_cold, + self, + ); self.simplify_candidate(&mut candidate); candidate }) diff --git a/compiler/rustc_mir_build/src/build/matches/test.rs b/compiler/rustc_mir_build/src/build/matches/test.rs index 2719ec7306692..1894cab83010f 100644 --- a/compiler/rustc_mir_build/src/build/matches/test.rs +++ b/compiler/rustc_mir_build/src/build/matches/test.rs @@ -219,7 +219,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { None } else { for i in cold_targets.iter() { - if *i < 2 { cold_br[*i] = true; } + if *i < 2 { + cold_br[*i] = true; + } } if cold_br[0] != cold_br[1] { Some(cold_br[0]) } else { None } }; @@ -231,7 +233,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { 0 => (second_bb, first_bb, cold_br.and_then(|x| Some(!x))), v => span_bug!(test.span, "expected boolean value but got {:?}", v), }; - TerminatorKind::if_with_cold_br(Operand::Copy(place), true_bb, false_bb, cold_br) + TerminatorKind::if_with_cold_br( + Operand::Copy(place), + true_bb, + false_bb, + cold_br, + ) } else { // The switch may be inexhaustive so we have a catch all block debug_assert_eq!(options.len() + 1, target_blocks.len()); From b8ffc96622b5c6bf79c95a5a174b13a8e4ac89e2 Mon Sep 17 00:00:00 2001 From: Jiri Bobek Date: Mon, 22 Jan 2024 23:51:42 +0100 Subject: [PATCH 4/8] removed redundant functions in the codegen --- compiler/rustc_codegen_llvm/src/builder.rs | 15 ++----------- compiler/rustc_codegen_llvm/src/va_arg.rs | 6 ++--- compiler/rustc_codegen_ssa/src/mir/block.rs | 12 +++++----- .../rustc_codegen_ssa/src/traits/builder.rs | 6 ----- compiler/rustc_middle/src/mir/terminator.rs | 22 ++++--------------- .../rustc_mir_build/src/build/matches/mod.rs | 2 +- .../rustc_mir_build/src/build/matches/test.rs | 11 +++------- .../rustc_mir_dataflow/src/elaborate_drops.rs | 4 ++-- .../rustc_mir_transform/src/coverage/tests.rs | 2 +- .../src/early_otherwise_branch.rs | 2 +- 10 files changed, 23 insertions(+), 59 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/builder.rs b/compiler/rustc_codegen_llvm/src/builder.rs index a2aead1228053..0defefce0551a 100644 --- a/compiler/rustc_codegen_llvm/src/builder.rs +++ b/compiler/rustc_codegen_llvm/src/builder.rs @@ -191,17 +191,6 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { cond: &'ll Value, then_llbb: &'ll BasicBlock, else_llbb: &'ll BasicBlock, - ) { - unsafe { - llvm::LLVMBuildCondBr(self.llbuilder, cond, then_llbb, else_llbb); - } - } - - fn cond_br_with_cold_br( - &mut self, - cond: &'ll Value, - then_llbb: Self::BasicBlock, - else_llbb: Self::BasicBlock, cold_br: Option, ) { // emit the branch instruction @@ -637,7 +626,7 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { let i = header_bx.phi(self.val_ty(zero), &[zero], &[self.llbb()]); let keep_going = header_bx.icmp(IntPredicate::IntULT, i, count); - header_bx.cond_br(keep_going, body_bb, next_bb); + header_bx.cond_br(keep_going, body_bb, next_bb, None); let mut body_bx = Self::build(self.cx, body_bb); let dest_elem = dest.project_index(&mut body_bx, i); @@ -1588,7 +1577,7 @@ impl<'a, 'll, 'tcx> Builder<'a, 'll, 'tcx> { let cond = self.type_test(llfn, typeid_metadata); let bb_pass = self.append_sibling_block("type_test.pass"); let bb_fail = self.append_sibling_block("type_test.fail"); - self.cond_br(cond, bb_pass, bb_fail); + self.cond_br(cond, bb_pass, bb_fail, None); self.switch_to_block(bb_fail); self.abort(); diff --git a/compiler/rustc_codegen_llvm/src/va_arg.rs b/compiler/rustc_codegen_llvm/src/va_arg.rs index 172c66a7af13c..520cc3c932b48 100644 --- a/compiler/rustc_codegen_llvm/src/va_arg.rs +++ b/compiler/rustc_codegen_llvm/src/va_arg.rs @@ -119,7 +119,7 @@ fn emit_aapcs_va_arg<'ll, 'tcx>( // if the offset >= 0 then the value will be on the stack let mut reg_off_v = bx.load(bx.type_i32(), reg_off, offset_align); let use_stack = bx.icmp(IntPredicate::IntSGE, reg_off_v, zero); - bx.cond_br(use_stack, on_stack, maybe_reg); + bx.cond_br(use_stack, on_stack, maybe_reg, None); // The value at this point might be in a register, but there is a chance that // it could be on the stack so we have to update the offset and then check @@ -137,7 +137,7 @@ fn emit_aapcs_va_arg<'ll, 'tcx>( // Check to see if we have overflowed the registers as a result of this. // If we have then we need to use the stack for this value let use_stack = bx.icmp(IntPredicate::IntSGT, new_reg_off_v, zero); - bx.cond_br(use_stack, on_stack, in_reg); + bx.cond_br(use_stack, on_stack, in_reg, None); bx.switch_to_block(in_reg); let top_type = bx.type_ptr(); @@ -203,7 +203,7 @@ fn emit_s390x_va_arg<'ll, 'tcx>( ); let reg_count_v = bx.load(bx.type_i64(), reg_count, Align::from_bytes(8).unwrap()); let use_regs = bx.icmp(IntPredicate::IntULT, reg_count_v, bx.const_u64(max_regs)); - bx.cond_br(use_regs, in_reg, in_mem); + bx.cond_br(use_regs, in_reg, in_mem, None); // Emit code to load the value if it was passed in a register. bx.switch_to_block(in_reg); diff --git a/compiler/rustc_codegen_ssa/src/mir/block.rs b/compiler/rustc_codegen_ssa/src/mir/block.rs index 1193bd7b2736d..6515c4bf76625 100644 --- a/compiler/rustc_codegen_ssa/src/mir/block.rs +++ b/compiler/rustc_codegen_ssa/src/mir/block.rs @@ -335,16 +335,16 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { match test_value { 0 => { let cold_br = cold_br.and_then(|t| Some(!t)); - bx.cond_br_with_cold_br(discr.immediate(), llfalse, lltrue, cold_br); + bx.cond_br(discr.immediate(), llfalse, lltrue, cold_br); } - 1 => bx.cond_br_with_cold_br(discr.immediate(), lltrue, llfalse, cold_br), + 1 => bx.cond_br(discr.immediate(), lltrue, llfalse, cold_br), _ => bug!(), } } else { let switch_llty = bx.immediate_backend_type(bx.layout_of(switch_ty)); let llval = bx.const_uint_big(switch_llty, test_value); let cmp = bx.icmp(IntPredicate::IntEQ, discr.immediate(), llval); - bx.cond_br_with_cold_br(cmp, lltrue, llfalse, cold_br); + bx.cond_br(cmp, lltrue, llfalse, cold_br); } } else if self.cx.sess().opts.optimize == OptLevel::No && target_iter.len() == 2 @@ -369,7 +369,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { let switch_llty = bx.immediate_backend_type(bx.layout_of(switch_ty)); let llval = bx.const_uint_big(switch_llty, test_value1); let cmp = bx.icmp(IntPredicate::IntEQ, discr.immediate(), llval); - bx.cond_br(cmp, ll1, ll2); + bx.cond_br(cmp, ll1, ll2, None); } else { bx.switch( discr.immediate(), @@ -601,9 +601,9 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { let lltarget = helper.llbb_with_cleanup(self, target); let panic_block = bx.append_sibling_block("panic"); if expected { - bx.cond_br(cond, lltarget, panic_block); + bx.cond_br(cond, lltarget, panic_block, None); } else { - bx.cond_br(cond, panic_block, lltarget); + bx.cond_br(cond, panic_block, lltarget, None); } // After this point, bx is the block for the call to panic. diff --git a/compiler/rustc_codegen_ssa/src/traits/builder.rs b/compiler/rustc_codegen_ssa/src/traits/builder.rs index 9e8ccdd023452..4c706d723a153 100644 --- a/compiler/rustc_codegen_ssa/src/traits/builder.rs +++ b/compiler/rustc_codegen_ssa/src/traits/builder.rs @@ -63,12 +63,6 @@ pub trait BuilderMethods<'a, 'tcx>: cond: Self::Value, then_llbb: Self::BasicBlock, else_llbb: Self::BasicBlock, - ); - fn cond_br_with_cold_br( - &mut self, - cond: Self::Value, - then_llbb: Self::BasicBlock, - else_llbb: Self::BasicBlock, cold_br: Option, ); fn switch( diff --git a/compiler/rustc_middle/src/mir/terminator.rs b/compiler/rustc_middle/src/mir/terminator.rs index 4ce52b4afe583..bc8c4f5b8093d 100644 --- a/compiler/rustc_middle/src/mir/terminator.rs +++ b/compiler/rustc_middle/src/mir/terminator.rs @@ -20,20 +20,10 @@ impl SwitchTargets { Self { values, targets, cold_targets: ThinVec::new() } } - /// Builds a switch targets definition that jumps to `then` if the tested value equals `value`, - /// and to `else_` if not. - pub fn static_if(value: u128, then: BasicBlock, else_: BasicBlock) -> Self { - Self { - values: smallvec![value], - targets: smallvec![then, else_], - cold_targets: ThinVec::new(), - } - } - /// Builds a switch targets definition that jumps to `then` if the tested value equals `value`, /// and to `else_` if not. /// If cold_br is some bool value, the given outcome is considered cold (i.e., unlikely). - pub fn static_if_with_cold_br( + pub fn static_if( value: u128, then: BasicBlock, else_: BasicBlock, @@ -61,7 +51,7 @@ impl SwitchTargets { } } - // If this switch has exactly one target, returns it. + // If this switch has exactly one cold target, returns it. pub fn cold_target(&self) -> Option { if self.cold_targets.len() == 1 { Some(self.cold_targets[0]) } else { None } } @@ -390,11 +380,7 @@ impl<'tcx> Terminator<'tcx> { impl<'tcx> TerminatorKind<'tcx> { #[inline] - pub fn if_(cond: Operand<'tcx>, t: BasicBlock, f: BasicBlock) -> TerminatorKind<'tcx> { - TerminatorKind::SwitchInt { discr: cond, targets: SwitchTargets::static_if(0, f, t) } - } - - pub fn if_with_cold_br( + pub fn if_( cond: Operand<'tcx>, t: BasicBlock, f: BasicBlock, @@ -402,7 +388,7 @@ impl<'tcx> TerminatorKind<'tcx> { ) -> TerminatorKind<'tcx> { TerminatorKind::SwitchInt { discr: cond, - targets: SwitchTargets::static_if_with_cold_br( + targets: SwitchTargets::static_if( 0, f, t, diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 0e39d64a6a3c2..c040c5f07bcbd 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -174,7 +174,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let then_block = this.cfg.start_new_block(); let else_block = this.cfg.start_new_block(); let term = - TerminatorKind::if_with_cold_br(operand, then_block, else_block, cold_branch); + TerminatorKind::if_(operand, then_block, else_block, cold_branch); let source_info = this.source_info(expr_span); this.cfg.terminate(block, source_info, term); diff --git a/compiler/rustc_mir_build/src/build/matches/test.rs b/compiler/rustc_mir_build/src/build/matches/test.rs index 1894cab83010f..b71013fc43b05 100644 --- a/compiler/rustc_mir_build/src/build/matches/test.rs +++ b/compiler/rustc_mir_build/src/build/matches/test.rs @@ -233,12 +233,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { 0 => (second_bb, first_bb, cold_br.and_then(|x| Some(!x))), v => span_bug!(test.span, "expected boolean value but got {:?}", v), }; - TerminatorKind::if_with_cold_br( - Operand::Copy(place), - true_bb, - false_bb, - cold_br, - ) + TerminatorKind::if_(Operand::Copy(place), true_bb, false_bb, cold_br) } else { // The switch may be inexhaustive so we have a catch all block debug_assert_eq!(options.len() + 1, target_blocks.len()); @@ -419,7 +414,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { self.cfg.terminate( block, source_info, - TerminatorKind::if_(Operand::Move(result), success_block, fail_block), + TerminatorKind::if_(Operand::Move(result), success_block, fail_block, None), ); } @@ -562,7 +557,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { self.cfg.terminate( eq_block, source_info, - TerminatorKind::if_(Operand::Move(eq_result), success_block, fail_block), + TerminatorKind::if_(Operand::Move(eq_result), success_block, fail_block, None), ); } diff --git a/compiler/rustc_mir_dataflow/src/elaborate_drops.rs b/compiler/rustc_mir_dataflow/src/elaborate_drops.rs index 862876f53c76c..1a6dbaa65c385 100644 --- a/compiler/rustc_mir_dataflow/src/elaborate_drops.rs +++ b/compiler/rustc_mir_dataflow/src/elaborate_drops.rs @@ -733,7 +733,7 @@ where is_cleanup: unwind.is_cleanup(), terminator: Some(Terminator { source_info: self.source_info, - kind: TerminatorKind::if_(move_(can_go), succ, drop_block), + kind: TerminatorKind::if_(move_(can_go), succ, drop_block, None), }), }; let loop_block = self.elaborator.patch().new_block(loop_block); @@ -954,7 +954,7 @@ where DropStyle::Static => on_set, DropStyle::Conditional | DropStyle::Open => { let flag = self.elaborator.get_drop_flag(self.path).unwrap(); - let term = TerminatorKind::if_(flag, on_set, on_unset); + let term = TerminatorKind::if_(flag, on_set, on_unset, None); self.new_block(unwind, term) } } diff --git a/compiler/rustc_mir_transform/src/coverage/tests.rs b/compiler/rustc_mir_transform/src/coverage/tests.rs index d9a3c0cb162f3..0e2ea23242450 100644 --- a/compiler/rustc_mir_transform/src/coverage/tests.rs +++ b/compiler/rustc_mir_transform/src/coverage/tests.rs @@ -153,7 +153,7 @@ impl<'tcx> MockBlocks<'tcx> { fn switchint(&mut self, some_from_block: Option) -> BasicBlock { let switchint_kind = TerminatorKind::SwitchInt { discr: Operand::Move(Place::from(self.new_temp())), - targets: SwitchTargets::static_if(0, TEMP_BLOCK, TEMP_BLOCK), + targets: SwitchTargets::static_if(0, TEMP_BLOCK, TEMP_BLOCK, None), }; self.add_block_from(some_from_block, switchint_kind) } diff --git a/compiler/rustc_mir_transform/src/early_otherwise_branch.rs b/compiler/rustc_mir_transform/src/early_otherwise_branch.rs index 0d600f0f937de..0948a84cfc604 100644 --- a/compiler/rustc_mir_transform/src/early_otherwise_branch.rs +++ b/compiler/rustc_mir_transform/src/early_otherwise_branch.rs @@ -191,7 +191,7 @@ impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch { let false_case = eq_bb; patch.patch_terminator( parent, - TerminatorKind::if_(Operand::Move(Place::from(comp_temp)), true_case, false_case), + TerminatorKind::if_(Operand::Move(Place::from(comp_temp)), true_case, false_case, None), ); // generate StorageDead for the second_discriminant_temp not in use anymore From 3aef65d2a04fc76e9a3b02b7b79a738a5b745a00 Mon Sep 17 00:00:00 2001 From: Jiri Bobek Date: Mon, 22 Jan 2024 23:55:13 +0100 Subject: [PATCH 5/8] formating change to pass 'test tidy' --- compiler/rustc_mir_build/src/build/matches/mod.rs | 3 +-- compiler/rustc_mir_transform/src/early_otherwise_branch.rs | 7 ++++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index c040c5f07bcbd..73f7299aa8293 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -173,8 +173,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let then_block = this.cfg.start_new_block(); let else_block = this.cfg.start_new_block(); - let term = - TerminatorKind::if_(operand, then_block, else_block, cold_branch); + let term = TerminatorKind::if_(operand, then_block, else_block, cold_branch); let source_info = this.source_info(expr_span); this.cfg.terminate(block, source_info, term); diff --git a/compiler/rustc_mir_transform/src/early_otherwise_branch.rs b/compiler/rustc_mir_transform/src/early_otherwise_branch.rs index 0948a84cfc604..83a8df9da1a20 100644 --- a/compiler/rustc_mir_transform/src/early_otherwise_branch.rs +++ b/compiler/rustc_mir_transform/src/early_otherwise_branch.rs @@ -191,7 +191,12 @@ impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch { let false_case = eq_bb; patch.patch_terminator( parent, - TerminatorKind::if_(Operand::Move(Place::from(comp_temp)), true_case, false_case, None), + TerminatorKind::if_( + Operand::Move(Place::from(comp_temp)), + true_case, + false_case, + None, + ), ); // generate StorageDead for the second_discriminant_temp not in use anymore From 9d7d8468bae630cad09832a035c051e13c01fa23 Mon Sep 17 00:00:00 2001 From: Jiri Bobek Date: Tue, 23 Jan 2024 00:32:00 +0100 Subject: [PATCH 6/8] initial tests --- tests/codegen/cold_match_arms/cold_false.rs | 30 +++++++++++++++++++++ tests/codegen/cold_match_arms/cold_true.rs | 30 +++++++++++++++++++++ 2 files changed, 60 insertions(+) create mode 100644 tests/codegen/cold_match_arms/cold_false.rs create mode 100644 tests/codegen/cold_match_arms/cold_true.rs diff --git a/tests/codegen/cold_match_arms/cold_false.rs b/tests/codegen/cold_match_arms/cold_false.rs new file mode 100644 index 0000000000000..4d269bddb4e77 --- /dev/null +++ b/tests/codegen/cold_match_arms/cold_false.rs @@ -0,0 +1,30 @@ +// compile-flags: -O +#![crate_type = "lib"] + +#[inline(never)] +#[no_mangle] +pub fn hot_function() { + println!("hot"); +} + +#[inline(never)] +#[no_mangle] +pub fn cold_function() { + println!("cold"); +} + +#[no_mangle] +pub fn f(x: bool) { + match x { + true => hot_function(), + #[cold] false => cold_function(), + } +} + +// CHECK-LABEL: @f( +// CHECK: br i1 %x, label %bb2, label %bb1, !prof ![[NUM:[0-9]+]] +// CHECK: bb1: +// CHECK: cold_function +// CHECK: bb2: +// CHECK: hot_function +// CHECK: ![[NUM]] = !{!"branch_weights", i32 2000, i32 1} diff --git a/tests/codegen/cold_match_arms/cold_true.rs b/tests/codegen/cold_match_arms/cold_true.rs new file mode 100644 index 0000000000000..58b09884de512 --- /dev/null +++ b/tests/codegen/cold_match_arms/cold_true.rs @@ -0,0 +1,30 @@ +// compile-flags: -O +#![crate_type = "lib"] + +#[inline(never)] +#[no_mangle] +pub fn hot_function() { + println!("hot"); +} + +#[inline(never)] +#[no_mangle] +pub fn cold_function() { + println!("cold"); +} + +#[no_mangle] +pub fn f(x: bool) { + match x { + #[cold] true => cold_function(), + false => hot_function(), + } +} + +// CHECK-LABEL: @f( +// CHECK: br i1 %x, label %bb2, label %bb1, !prof ![[NUM:[0-9]+]] +// CHECK: bb1: +// CHECK: hot_function +// CHECK: bb2: +// CHECK: cold_function +// CHECK: ![[NUM]] = !{!"branch_weights", i32 1, i32 2000} From e8b9ad557ae08a8082b848f1fe3cab1975bde8c4 Mon Sep 17 00:00:00 2001 From: Jiri Bobek Date: Sat, 3 Feb 2024 10:22:07 +0100 Subject: [PATCH 7/8] added comments --- .../rustc_mir_build/src/build/matches/mod.rs | 30 ++++++++++++++----- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 73f7299aa8293..4f0f301de31f8 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -56,6 +56,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { match expr.kind { ExprKind::LogicalOp { op: LogicalOp::And, lhs, rhs } => { + // cold_branch == true is equivalent to unlikely(lhs && rhs) + // cold_branch == false is equivalent to likely(lhs && rhs) + // We propagate the expectation to the lhs and rhs operands as follows: + // likely(lhs && rhs) => likely(lhs) && likely(rhs) + // unlikely(lhs && rhs) => lhs && unlikely(rhs) + // Note that the `unlikely` propagation may be incorrect. With the information + // available, we can't tell which of the operands in unlikely. + let lhs_then_block = unpack!(this.then_else_break( block, lhs, @@ -64,8 +72,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { variable_source_info, declare_bindings, match cold_branch { - Some(false) => Some(false), - _ => None, + Some(false) => Some(false), // propagate likely + _ => None, // but not unlikely }, )); @@ -76,12 +84,20 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { break_scope, variable_source_info, declare_bindings, - cold_branch, + cold_branch, // propagate both likely and unlikely )); rhs_then_block.unit() } ExprKind::LogicalOp { op: LogicalOp::Or, lhs, rhs } => { + // cold_branch == true is equivalent to unlikely(lhs || rhs) + // cold_branch == false is equivalent to likely(lhs || rhs) + // We propagate the expectation to the lhs and rhs operands as follows: + // likely(lhs || rhs) => lhs || likely(rhs) + // unlikely(lhs || rhs) => unlikely(lhs) && unlikely(rhs) + // Note that the `likely` propagation may be incorrect. With the information + // available, we can't tell which of the operands in likely. + let local_scope = this.local_scope(); let (lhs_success_block, failure_block) = this.in_if_then_scope(local_scope, expr_span, |this| { @@ -93,8 +109,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { variable_source_info, true, match cold_branch { - Some(true) => Some(true), - _ => None, + Some(true) => Some(true), // propagate unlikely + _ => None, // but not likely }, ) }); @@ -105,7 +121,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { break_scope, variable_source_info, true, - cold_branch, + cold_branch, // propagate both likely and unlikely )); this.cfg.goto(lhs_success_block, variable_source_info, rhs_success_block); rhs_success_block.unit() @@ -126,7 +142,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { local_scope, variable_source_info, true, - cold_branch.and_then(|b| Some(!b)), + cold_branch.and_then(|b| Some(!b)), // propagate the opposite of likely/unlikely ) }); this.break_for_else(success_block, break_scope, variable_source_info); From 7472434853cf6313f2aa38f2bef75c77f4813d62 Mon Sep 17 00:00:00 2001 From: Jiri Bobek Date: Sat, 3 Feb 2024 10:36:10 +0100 Subject: [PATCH 8/8] typo --- compiler/rustc_mir_build/src/build/matches/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 4f0f301de31f8..10332fde152e9 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -62,7 +62,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // likely(lhs && rhs) => likely(lhs) && likely(rhs) // unlikely(lhs && rhs) => lhs && unlikely(rhs) // Note that the `unlikely` propagation may be incorrect. With the information - // available, we can't tell which of the operands in unlikely. + // available, we can't tell which of the operands is unlikely. let lhs_then_block = unpack!(this.then_else_break( block, @@ -96,7 +96,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // likely(lhs || rhs) => lhs || likely(rhs) // unlikely(lhs || rhs) => unlikely(lhs) && unlikely(rhs) // Note that the `likely` propagation may be incorrect. With the information - // available, we can't tell which of the operands in likely. + // available, we can't tell which of the operands is likely. let local_scope = this.local_scope(); let (lhs_success_block, failure_block) =