From 75a39a8d7bdd9789e3a6fde0f18177d3a87223f6 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 29 Oct 2019 08:24:59 -0700 Subject: [PATCH 1/5] Add method to `Candidate` that determines its promotability rules --- src/librustc_mir/transform/promote_consts.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/librustc_mir/transform/promote_consts.rs b/src/librustc_mir/transform/promote_consts.rs index 3af08090853a6..4859b7e0a0bad 100644 --- a/src/librustc_mir/transform/promote_consts.rs +++ b/src/librustc_mir/transform/promote_consts.rs @@ -80,6 +80,17 @@ pub enum Candidate { Argument { bb: BasicBlock, index: usize }, } +impl Candidate { + /// Returns `true` if we should use the "explicit" rules for promotability for this `Candidate`. + fn is_explicit_context(&self) -> bool { + match self { + Candidate::Ref(_) | + Candidate::Repeat(_) => false, + Candidate::Argument { .. } => true, + } + } +} + fn args_required_const(tcx: TyCtxt<'_>, def_id: DefId) -> Option> { let attrs = tcx.get_attrs(def_id); let attr = attrs.iter().find(|a| a.check_name(sym::rustc_args_required_const))?; @@ -727,11 +738,7 @@ pub fn validate_candidates( }; candidates.iter().copied().filter(|&candidate| { - validator.explicit = match candidate { - Candidate::Ref(_) | - Candidate::Repeat(_) => false, - Candidate::Argument { .. } => true, - }; + validator.explicit = candidate.is_explicit_context(); // FIXME(eddyb) also emit the errors for shuffle indices // and `#[rustc_args_required_const]` arguments here. From 605b0d82d511e0bdf4d086c924e9e4fb1d3cc7fa Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 29 Oct 2019 09:57:00 -0700 Subject: [PATCH 2/5] Set explicit flag inside `validate_candidate` --- src/librustc_mir/transform/promote_consts.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/librustc_mir/transform/promote_consts.rs b/src/librustc_mir/transform/promote_consts.rs index 4859b7e0a0bad..7375a036c8811 100644 --- a/src/librustc_mir/transform/promote_consts.rs +++ b/src/librustc_mir/transform/promote_consts.rs @@ -261,11 +261,11 @@ impl std::ops::Deref for Validator<'a, 'tcx> { struct Unpromotable; impl<'tcx> Validator<'_, 'tcx> { - fn validate_candidate(&self, candidate: Candidate) -> Result<(), Unpromotable> { + fn validate_candidate(&mut self, candidate: Candidate) -> Result<(), Unpromotable> { + self.explicit = candidate.is_explicit_context(); + match candidate { Candidate::Ref(loc) => { - assert!(!self.explicit); - let statement = &self.body[loc.block].statements[loc.statement_index]; match &statement.kind { StatementKind::Assign(box(_, Rvalue::Ref(_, kind, place))) => { @@ -356,8 +356,6 @@ impl<'tcx> Validator<'_, 'tcx> { } } Candidate::Repeat(loc) => { - assert!(!self.explicit); - let statement = &self.body[loc.block].statements[loc.statement_index]; match &statement.kind { StatementKind::Assign(box(_, Rvalue::Repeat(ref operand, _))) => { @@ -371,8 +369,6 @@ impl<'tcx> Validator<'_, 'tcx> { } }, Candidate::Argument { bb, index } => { - assert!(self.explicit); - let terminator = self.body[bb].terminator(); match &terminator.kind { TerminatorKind::Call { args, .. } => { @@ -738,8 +734,6 @@ pub fn validate_candidates( }; candidates.iter().copied().filter(|&candidate| { - validator.explicit = candidate.is_explicit_context(); - // FIXME(eddyb) also emit the errors for shuffle indices // and `#[rustc_args_required_const]` arguments here. From 613c4b7a264b346dfa3c1f6e4484f3fa8da844ed Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 29 Oct 2019 09:57:44 -0700 Subject: [PATCH 3/5] Emit errors in `promote_consts` when explicit promotion fails --- src/librustc_mir/transform/promote_consts.rs | 129 +++++++++++-------- src/librustc_mir/transform/qualify_consts.rs | 24 ++-- 2 files changed, 88 insertions(+), 65 deletions(-) diff --git a/src/librustc_mir/transform/promote_consts.rs b/src/librustc_mir/transform/promote_consts.rs index 7375a036c8811..2ed0f4efa2324 100644 --- a/src/librustc_mir/transform/promote_consts.rs +++ b/src/librustc_mir/transform/promote_consts.rs @@ -104,15 +104,12 @@ fn args_required_const(tcx: TyCtxt<'_>, def_id: DefId) -> Option> { Some(ret) } -struct Collector<'a, 'tcx> { - tcx: TyCtxt<'tcx>, +struct TempStateCollector<'a, 'tcx> { body: &'a Body<'tcx>, temps: IndexVec, - candidates: Vec, - span: Span, } -impl<'tcx> Visitor<'tcx> for Collector<'_, 'tcx> { +impl<'tcx> Visitor<'tcx> for TempStateCollector<'_, 'tcx> { fn visit_local(&mut self, &index: &Local, context: PlaceContext, @@ -166,18 +163,34 @@ impl<'tcx> Visitor<'tcx> for Collector<'_, 'tcx> { } *temp = TempState::Unpromotable; } +} + +struct PromotionCandidateCollector<'mir, 'tcx> { + validator: Validator<'mir, 'tcx>, + candidates: Vec, + span: Span, +} +impl<'tcx> Visitor<'tcx> for PromotionCandidateCollector<'_, 'tcx> { fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, location: Location) { self.super_rvalue(rvalue, location); + let tcx = self.validator.tcx; + match *rvalue { Rvalue::Ref(..) => { - self.candidates.push(Candidate::Ref(location)); + let candidate = Candidate::Ref(location); + if self.validator.validate_candidate(candidate).is_ok() { + self.candidates.push(candidate); + } } - Rvalue::Repeat(..) if self.tcx.features().const_in_array_repeat_expressions => { + Rvalue::Repeat(..) if tcx.features().const_in_array_repeat_expressions => { // FIXME(#49147) only promote the element when it isn't `Copy` // (so that code that can copy it at runtime is unaffected). - self.candidates.push(Candidate::Repeat(location)); + let candidate = Candidate::Repeat(location); + if self.validator.validate_candidate(candidate).is_ok() { + self.candidates.push(candidate); + } } _ => {} } @@ -188,23 +201,40 @@ impl<'tcx> Visitor<'tcx> for Collector<'_, 'tcx> { location: Location) { self.super_terminator_kind(kind, location); + let Item { tcx, body, .. } = *self.validator.item; + if let TerminatorKind::Call { ref func, .. } = *kind { - if let ty::FnDef(def_id, _) = func.ty(self.body, self.tcx).kind { - let fn_sig = self.tcx.fn_sig(def_id); + if let ty::FnDef(def_id, _) = func.ty(body, tcx).kind { + let fn_sig = tcx.fn_sig(def_id); if let Abi::RustIntrinsic | Abi::PlatformIntrinsic = fn_sig.abi() { - let name = self.tcx.item_name(def_id); + let name = tcx.item_name(def_id); // FIXME(eddyb) use `#[rustc_args_required_const(2)]` for shuffles. if name.as_str().starts_with("simd_shuffle") { - self.candidates.push(Candidate::Argument { - bb: location.block, - index: 2, - }); + let candidate = Candidate::Argument { bb: location.block, index: 2 }; + + if self.validator.validate_candidate(candidate).is_ok() { + self.candidates.push(candidate); + } else { + span_err!( + tcx.sess, self.span, E0526, + "shuffle indices are not constant", + ); + } } } - if let Some(constant_args) = args_required_const(self.tcx, def_id) { + if let Some(constant_args) = args_required_const(tcx, def_id) { for index in constant_args { - self.candidates.push(Candidate::Argument { bb: location.block, index }); + let candidate = Candidate::Argument { bb: location.block, index }; + + if self.validator.validate_candidate(candidate).is_ok() { + self.candidates.push(candidate); + } else { + tcx.sess.span_err( + self.span, + &format!("argument {} is required to be a constant", index + 1), + ); + } } } } @@ -216,29 +246,45 @@ impl<'tcx> Visitor<'tcx> for Collector<'_, 'tcx> { } } -pub fn collect_temps_and_candidates( - tcx: TyCtxt<'tcx>, - body: &Body<'tcx>, - rpo: &mut ReversePostorder<'_, 'tcx>, +pub fn collect_temps_and_valid_candidates( + item: &Item<'_, 'tcx>, + mut rpo: &mut ReversePostorder<'_, 'tcx>, ) -> (IndexVec, Vec) { - let mut collector = Collector { - tcx, - body, - temps: IndexVec::from_elem(TempState::Undefined, &body.local_decls), + let mut temp_state = TempStateCollector { + body: item.body, + temps: IndexVec::from_elem(TempState::Undefined, &item.body.local_decls), + }; + + for (bb, data) in &mut rpo { + temp_state.visit_basic_block_data(bb, data); + } + + let mut promotion = PromotionCandidateCollector { + span: item.body.span, candidates: vec![], - span: body.span, + validator: Validator { + item, + temps: &temp_state.temps, + explicit: false, + }, }; - for (bb, data) in rpo { - collector.visit_basic_block_data(bb, data); + + // FIXME: We need to visit blocks in RPO above to record all uses, but we probably don't need + // to do the same here? What happens if a BB is not visited during RPO traversal? I've observed + // this occur for generators. + for (bb, data) in &mut rpo { + promotion.visit_basic_block_data(bb, data); } - (collector.temps, collector.candidates) + + let candidates = promotion.candidates; + (temp_state.temps, candidates) } /// Checks whether locals that appear in a promotion context (`Candidate`) are actually promotable. /// /// This wraps an `Item`, and has access to all fields of that `Item` via `Deref` coercion. struct Validator<'a, 'tcx> { - item: Item<'a, 'tcx>, + item: &'a Item<'a, 'tcx>, temps: &'a IndexVec, /// Explicit promotion happens e.g. for constant arguments declared via @@ -260,6 +306,7 @@ impl std::ops::Deref for Validator<'a, 'tcx> { struct Unpromotable; +// FIXME(eddyb) remove the differences for promotability in `static`, `const`, `const fn`. impl<'tcx> Validator<'_, 'tcx> { fn validate_candidate(&mut self, candidate: Candidate) -> Result<(), Unpromotable> { self.explicit = candidate.is_explicit_context(); @@ -719,28 +766,6 @@ impl<'tcx> Validator<'_, 'tcx> { } } -// FIXME(eddyb) remove the differences for promotability in `static`, `const`, `const fn`. -pub fn validate_candidates( - tcx: TyCtxt<'tcx>, - body: &Body<'tcx>, - def_id: DefId, - temps: &IndexVec, - candidates: &[Candidate], -) -> Vec { - let mut validator = Validator { - item: Item::new(tcx, def_id, body), - temps, - explicit: false, - }; - - candidates.iter().copied().filter(|&candidate| { - // FIXME(eddyb) also emit the errors for shuffle indices - // and `#[rustc_args_required_const]` arguments here. - - validator.validate_candidate(candidate).is_ok() - }).collect() -} - struct Promoter<'a, 'tcx> { tcx: TyCtxt<'tcx>, source: &'a mut Body<'tcx>, diff --git a/src/librustc_mir/transform/qualify_consts.rs b/src/librustc_mir/transform/qualify_consts.rs index 2f77cd5ddf716..d941725394d5e 100644 --- a/src/librustc_mir/transform/qualify_consts.rs +++ b/src/librustc_mir/transform/qualify_consts.rs @@ -678,7 +678,7 @@ struct Checker<'a, 'tcx> { temp_promotion_state: IndexVec, promotion_candidates: Vec, - unchecked_promotion_candidates: Vec, + checked_promotion_candidates: Vec, /// If `true`, do not emit errors to the user, merely collect them in `errors`. suppress_errors: bool, @@ -706,10 +706,14 @@ impl Deref for Checker<'a, 'tcx> { impl<'a, 'tcx> Checker<'a, 'tcx> { fn new(tcx: TyCtxt<'tcx>, def_id: DefId, body: &'a Body<'tcx>, mode: Mode) -> Self { + use crate::transform::check_consts as new_checker; + assert!(def_id.is_local()); + + let item = new_checker::Item::new(tcx, def_id, body); let mut rpo = traversal::reverse_postorder(body); - let (temps, unchecked_promotion_candidates) = - promote_consts::collect_temps_and_candidates(tcx, body, &mut rpo); + let (temps, checked_promotion_candidates) = + promote_consts::collect_temps_and_valid_candidates(&item, &mut rpo); rpo.reset(); let param_env = tcx.param_env(def_id); @@ -748,7 +752,7 @@ impl<'a, 'tcx> Checker<'a, 'tcx> { rpo, temp_promotion_state: temps, promotion_candidates: vec![], - unchecked_promotion_candidates, + checked_promotion_candidates, errors: vec![], suppress_errors: false, } @@ -1107,18 +1111,12 @@ impl<'a, 'tcx> Checker<'a, 'tcx> { (qualifs.encode_to_bits(), self.tcx.arena.alloc(promoted_temps)) } - /// Get the subset of `unchecked_promotion_candidates` that are eligible - /// for promotion. + /// Get the set of promotion candidates that the new promoter has deemed eligible for + /// promotion. These will be compared against the candidates generated above in `Checker`. // FIXME(eddyb) replace the old candidate gathering with this. fn valid_promotion_candidates(&self) -> Vec { // Sanity-check the promotion candidates. - let candidates = promote_consts::validate_candidates( - self.tcx, - self.body, - self.def_id, - &self.temp_promotion_state, - &self.unchecked_promotion_candidates, - ); + let candidates = self.checked_promotion_candidates.clone(); if candidates != self.promotion_candidates { let report = |msg, candidate| { From 16027096f39707a8e60215fee01bf6c62ca59ec2 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 29 Oct 2019 11:06:44 -0700 Subject: [PATCH 4/5] Promote the same basic blocks as `qualify_consts` does --- src/librustc_mir/transform/promote_consts.rs | 56 ++++++++++++++++++-- src/librustc_mir/transform/qualify_consts.rs | 22 +------- 2 files changed, 52 insertions(+), 26 deletions(-) diff --git a/src/librustc_mir/transform/promote_consts.rs b/src/librustc_mir/transform/promote_consts.rs index 2ed0f4efa2324..a5241fed1cfc7 100644 --- a/src/librustc_mir/transform/promote_consts.rs +++ b/src/librustc_mir/transform/promote_consts.rs @@ -25,6 +25,7 @@ use syntax::symbol::sym; use syntax_pos::{Span, DUMMY_SP}; use rustc_index::vec::{IndexVec, Idx}; +use rustc_index::bit_set::BitSet; use rustc_target::spec::abi::Abi; use std::{iter, mem, usize}; @@ -269,11 +270,56 @@ pub fn collect_temps_and_valid_candidates( }, }; - // FIXME: We need to visit blocks in RPO above to record all uses, but we probably don't need - // to do the same here? What happens if a BB is not visited during RPO traversal? I've observed - // this occur for generators. - for (bb, data) in &mut rpo { - promotion.visit_basic_block_data(bb, data); + if item.const_kind.is_none() { + rpo.reset(); + for (bb, data) in &mut rpo { + promotion.visit_basic_block_data(bb, data); + } + } else { + // FIXME: This strange MIR traversal is a temporary measure to ensure that we try to + // promote the same things as the old promotion logic in `qualify_consts`. + let mut seen_blocks = BitSet::new_empty(item.body.basic_blocks().len()); + let mut bb = START_BLOCK; + loop { + seen_blocks.insert(bb.index()); + + promotion.visit_basic_block_data(bb, &item.body[bb]); + + let target = match item.body[bb].terminator().kind { + TerminatorKind::Goto { target } | + TerminatorKind::FalseUnwind { real_target: target, .. } | + TerminatorKind::Drop { target, .. } | + TerminatorKind::DropAndReplace { target, .. } | + TerminatorKind::Assert { target, .. } | + TerminatorKind::Call { destination: Some((_, target)), .. } => { + Some(target) + } + + // Non-terminating calls cannot produce any value. + TerminatorKind::Call { destination: None, .. } => { + break; + } + + TerminatorKind::SwitchInt {..} | + TerminatorKind::Resume | + TerminatorKind::Abort | + TerminatorKind::GeneratorDrop | + TerminatorKind::Yield { .. } | + TerminatorKind::Unreachable | + TerminatorKind::FalseEdges { .. } => None, + + TerminatorKind::Return => { + break; + } + }; + + match target { + Some(target) if !seen_blocks.contains(target.index()) => { + bb = target; + } + _ => break, + } + } } let candidates = promotion.candidates; diff --git a/src/librustc_mir/transform/qualify_consts.rs b/src/librustc_mir/transform/qualify_consts.rs index d941725394d5e..8dd7b7ec56dcd 100644 --- a/src/librustc_mir/transform/qualify_consts.rs +++ b/src/librustc_mir/transform/qualify_consts.rs @@ -981,7 +981,6 @@ impl<'a, 'tcx> Checker<'a, 'tcx> { let mut seen_blocks = BitSet::new_empty(body.basic_blocks().len()); let mut bb = START_BLOCK; - let mut has_controlflow_error = false; loop { seen_blocks.insert(bb.index()); @@ -1022,7 +1021,6 @@ impl<'a, 'tcx> Checker<'a, 'tcx> { bb = target; } _ => { - has_controlflow_error = true; self.not_const(ops::Loop); validator.check_op(ops::Loop); break; @@ -1052,25 +1050,7 @@ impl<'a, 'tcx> Checker<'a, 'tcx> { // Collect all the temps we need to promote. let mut promoted_temps = BitSet::new_empty(self.temp_promotion_state.len()); - // HACK(eddyb) don't try to validate promotion candidates if any - // parts of the control-flow graph were skipped due to an error. - let promotion_candidates = if has_controlflow_error { - let unleash_miri = self - .tcx - .sess - .opts - .debugging_opts - .unleash_the_miri_inside_of_you; - if !unleash_miri { - self.tcx.sess.delay_span_bug( - body.span, - "check_const: expected control-flow error(s)", - ); - } - self.promotion_candidates.clone() - } else { - self.valid_promotion_candidates() - }; + let promotion_candidates = self.valid_promotion_candidates(); debug!("qualify_const: promotion_candidates={:?}", promotion_candidates); for candidate in promotion_candidates { match candidate { From d99b7b43d6c95c7eca2ab4c2ff9375896d275659 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 29 Oct 2019 11:21:32 -0700 Subject: [PATCH 5/5] Consider the const context when determining `explicit` --- src/librustc_mir/transform/promote_consts.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/transform/promote_consts.rs b/src/librustc_mir/transform/promote_consts.rs index a5241fed1cfc7..579f4e9e902ce 100644 --- a/src/librustc_mir/transform/promote_consts.rs +++ b/src/librustc_mir/transform/promote_consts.rs @@ -355,7 +355,8 @@ struct Unpromotable; // FIXME(eddyb) remove the differences for promotability in `static`, `const`, `const fn`. impl<'tcx> Validator<'_, 'tcx> { fn validate_candidate(&mut self, candidate: Candidate) -> Result<(), Unpromotable> { - self.explicit = candidate.is_explicit_context(); + // Always use "explicit" rules for promotability inside a `const`, `static` or `const fn`. + self.explicit = candidate.is_explicit_context() || self.const_kind.is_some(); match candidate { Candidate::Ref(loc) => { @@ -781,7 +782,7 @@ impl<'tcx> Validator<'_, 'tcx> { ) -> Result<(), Unpromotable> { let fn_ty = callee.ty(self.body, self.tcx); - if !self.explicit && self.const_kind.is_none() { + if !self.explicit { if let ty::FnDef(def_id, _) = fn_ty.kind { // Never promote runtime `const fn` calls of // functions without `#[rustc_promotable]`.