From 8b097c4ce92869b012c36ba0c7c0a2a096a17849 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Thu, 4 Oct 2018 16:23:22 +0200 Subject: [PATCH 1/6] Don't try to promote already promoted out temporaries --- src/librustc_mir/transform/promote_consts.rs | 2 ++ src/librustc_mir/transform/qualify_consts.rs | 18 ++++++++++++++---- .../ui/consts/const-eval/double_promotion.rs | 17 +++++++++++++++++ 3 files changed, 33 insertions(+), 4 deletions(-) create mode 100644 src/test/ui/consts/const-eval/double_promotion.rs diff --git a/src/librustc_mir/transform/promote_consts.rs b/src/librustc_mir/transform/promote_consts.rs index 34339b0634194..bba9260f2b829 100644 --- a/src/librustc_mir/transform/promote_consts.rs +++ b/src/librustc_mir/transform/promote_consts.rs @@ -332,6 +332,8 @@ impl<'a, 'tcx> Promoter<'a, 'tcx> { let operand = Operand::Copy(promoted_place(ty, span)); mem::replace(&mut args[index], operand) } + // already promoted out + TerminatorKind::Goto { .. } => return, _ => bug!() } } diff --git a/src/librustc_mir/transform/qualify_consts.rs b/src/librustc_mir/transform/qualify_consts.rs index 17fe78d325cf1..4808f02c1dff4 100644 --- a/src/librustc_mir/transform/qualify_consts.rs +++ b/src/librustc_mir/transform/qualify_consts.rs @@ -812,7 +812,9 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { let fn_ty = func.ty(self.mir, self.tcx); let mut callee_def_id = None; - let (mut is_shuffle, mut is_const_fn) = (false, false); + let mut is_shuffle = false; + let mut is_const_fn = false; + let mut is_promotable_const_fn = false; if let ty::FnDef(def_id, _) = fn_ty.sty { callee_def_id = Some(def_id); match self.tcx.fn_sig(def_id).abi() { @@ -873,6 +875,9 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { // functions without #[rustc_promotable] if self.tcx.is_promotable_const_fn(def_id) { is_const_fn = true; + is_promotable_const_fn = true; + } else if self.tcx.is_const_fn(def_id) { + is_const_fn = true; } } else { // stable const fn or unstable const fns with their feature gate @@ -974,7 +979,9 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { if !constant_arguments.contains(&i) { return } - if this.qualif.is_empty() { + // if the argument requires a constant, we care about constness, not + // promotability + if (this.qualif - Qualif::NOT_PROMOTABLE).is_empty() { this.promotion_candidates.push(candidate); } else { this.tcx.sess.span_err(this.span, @@ -985,7 +992,11 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { } // non-const fn calls. - if !is_const_fn { + if is_const_fn { + if !is_promotable_const_fn && self.mode == Mode::Fn { + self.qualif = Qualif::NOT_PROMOTABLE; + } + } else { self.qualif = Qualif::NOT_CONST; if self.mode != Mode::Fn { self.tcx.sess.delay_span_bug( @@ -1003,7 +1014,6 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { // Be conservative about the returned value of a const fn. let tcx = self.tcx; let ty = dest.ty(self.mir, tcx).to_ty(tcx); - self.qualif = Qualif::empty(); self.add_type(ty); } self.assign(dest, location); diff --git a/src/test/ui/consts/const-eval/double_promotion.rs b/src/test/ui/consts/const-eval/double_promotion.rs new file mode 100644 index 0000000000000..0e75ea8e66b3c --- /dev/null +++ b/src/test/ui/consts/const-eval/double_promotion.rs @@ -0,0 +1,17 @@ +// compile-pass + +#![feature(const_fn, rustc_attrs)] + +#[rustc_args_required_const(0)] +pub const fn a(value: u8) -> u8 { + value +} + +#[rustc_args_required_const(0)] +pub fn b(_: u8) { + unimplemented!() +} + +fn main() { + let _ = b(a(0)); +} From f7629eff32cf23680074ae72144ed748139100fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20S=CC=B6c=CC=B6h=CC=B6n=CC=B6e=CC=B6i=CC=B6d=CC=B6?= =?UTF-8?q?e=CC=B6r=20Scherer?= Date: Thu, 25 Oct 2018 18:28:14 +0200 Subject: [PATCH 2/6] Explain a comment in more detail --- src/librustc_mir/transform/qualify_consts.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/transform/qualify_consts.rs b/src/librustc_mir/transform/qualify_consts.rs index 4808f02c1dff4..99ccf4f3cf5bb 100644 --- a/src/librustc_mir/transform/qualify_consts.rs +++ b/src/librustc_mir/transform/qualify_consts.rs @@ -979,8 +979,15 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { if !constant_arguments.contains(&i) { return } - // if the argument requires a constant, we care about constness, not - // promotability + // Since the argument is required to be constant, + // we care about constness, not promotability. + // If we checked for promotability, we'd miss out on + // the results of function calls (which are never promoted) + // This is not a problem, because the argument explicitly + // requests constness, in contrast to regular promotion + // which happens even without the user requesting it. + // We can error out with a hard error if the argument is not + // constant here. if (this.qualif - Qualif::NOT_PROMOTABLE).is_empty() { this.promotion_candidates.push(candidate); } else { From 4dc028743dad4d2d71842c2bb20029b412edaca7 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Thu, 25 Oct 2018 19:37:07 +0200 Subject: [PATCH 3/6] Explain why we can encounter a `Goto` terminator that we want to promote --- src/librustc_mir/transform/promote_consts.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/transform/promote_consts.rs b/src/librustc_mir/transform/promote_consts.rs index bba9260f2b829..1d45850a080d5 100644 --- a/src/librustc_mir/transform/promote_consts.rs +++ b/src/librustc_mir/transform/promote_consts.rs @@ -332,7 +332,13 @@ impl<'a, 'tcx> Promoter<'a, 'tcx> { let operand = Operand::Copy(promoted_place(ty, span)); mem::replace(&mut args[index], operand) } - // already promoted out + // We expected a `TerminatorKind::Call` for which we'd like to promote an + // argument. Since `qualify_consts` saw a `TerminatorKind::Call` here, but + // we are seeing a `Goto`, that means that the `promote_temps` method + // already promoted this call away entirely. This case occurs when calling + // a function requiring a constant argument and as that constant value + // providing a value whose computation contains another call to a function + // requiring a constant argument. TerminatorKind::Goto { .. } => return, _ => bug!() } From f4fe9b0a09d1347e26a7f95e78be57360e70c411 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Thu, 25 Oct 2018 19:39:01 +0200 Subject: [PATCH 4/6] Clarify exclusion comment further --- src/librustc_mir/transform/qualify_consts.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/transform/qualify_consts.rs b/src/librustc_mir/transform/qualify_consts.rs index 99ccf4f3cf5bb..1305d02a87b6a 100644 --- a/src/librustc_mir/transform/qualify_consts.rs +++ b/src/librustc_mir/transform/qualify_consts.rs @@ -982,7 +982,8 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { // Since the argument is required to be constant, // we care about constness, not promotability. // If we checked for promotability, we'd miss out on - // the results of function calls (which are never promoted) + // the results of function calls (which are never promoted + // in runtime code) // This is not a problem, because the argument explicitly // requests constness, in contrast to regular promotion // which happens even without the user requesting it. From ee7f4a27d324db13ce27df9d79f258e64086d8f1 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Thu, 25 Oct 2018 21:14:25 +0200 Subject: [PATCH 5/6] Grammar nit --- src/librustc_mir/transform/promote_consts.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/transform/promote_consts.rs b/src/librustc_mir/transform/promote_consts.rs index 1d45850a080d5..a27809ca1f2ec 100644 --- a/src/librustc_mir/transform/promote_consts.rs +++ b/src/librustc_mir/transform/promote_consts.rs @@ -333,8 +333,8 @@ impl<'a, 'tcx> Promoter<'a, 'tcx> { mem::replace(&mut args[index], operand) } // We expected a `TerminatorKind::Call` for which we'd like to promote an - // argument. Since `qualify_consts` saw a `TerminatorKind::Call` here, but - // we are seeing a `Goto`, that means that the `promote_temps` method + // argument. `qualify_consts` saw a `TerminatorKind::Call` here, but + // we are seeing a `Goto`. That means that the `promote_temps` method // already promoted this call away entirely. This case occurs when calling // a function requiring a constant argument and as that constant value // providing a value whose computation contains another call to a function From fd77500ed1d12dfdffff95e65530c2806a324f22 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Thu, 25 Oct 2018 21:18:22 +0200 Subject: [PATCH 6/6] Clear up nonpromotable const fn call qualification --- src/librustc_mir/transform/qualify_consts.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/librustc_mir/transform/qualify_consts.rs b/src/librustc_mir/transform/qualify_consts.rs index 1305d02a87b6a..e7d694ffba91c 100644 --- a/src/librustc_mir/transform/qualify_consts.rs +++ b/src/librustc_mir/transform/qualify_consts.rs @@ -1000,11 +1000,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { } // non-const fn calls. - if is_const_fn { - if !is_promotable_const_fn && self.mode == Mode::Fn { - self.qualif = Qualif::NOT_PROMOTABLE; - } - } else { + if !is_const_fn { self.qualif = Qualif::NOT_CONST; if self.mode != Mode::Fn { self.tcx.sess.delay_span_bug( @@ -1022,6 +1018,11 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> { // Be conservative about the returned value of a const fn. let tcx = self.tcx; let ty = dest.ty(self.mir, tcx).to_ty(tcx); + if is_const_fn && !is_promotable_const_fn && self.mode == Mode::Fn { + self.qualif = Qualif::NOT_PROMOTABLE; + } else { + self.qualif = Qualif::empty(); + } self.add_type(ty); } self.assign(dest, location);