From 9a1a3b9013221b90a9b02ecd23614b6e2fcf6a2d Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 4 Oct 2019 11:02:04 +1000 Subject: [PATCH 1/5] Combine two `match` arms in `specialize()`. This reduces the number of call sites for `constructor_intersects_pattern()` from two to one, which the next commit will take advantage of. --- src/librustc_mir/hair/pattern/_match.rs | 138 ++++++++++++------------ 1 file changed, 68 insertions(+), 70 deletions(-) diff --git a/src/librustc_mir/hair/pattern/_match.rs b/src/librustc_mir/hair/pattern/_match.rs index d3bc61a4dde56..cf8ef8c769985 100644 --- a/src/librustc_mir/hair/pattern/_match.rs +++ b/src/librustc_mir/hair/pattern/_match.rs @@ -434,6 +434,13 @@ enum Constructor<'tcx> { } impl<'tcx> Constructor<'tcx> { + fn is_slice(&self) -> bool { + match self { + Slice { .. } => true, + _ => false, + } + } + fn variant_index_for_adt<'a>( &self, cx: &MatchCheckCtxt<'a, 'tcx>, @@ -1766,85 +1773,76 @@ fn specialize<'p, 'a: 'p, 'tcx>( Some(smallvec![subpattern]) } - PatKind::Constant { value } => { - match *constructor { - Slice(..) => { - // we extract an `Option` for the pointer because slices of zero elements don't - // necessarily point to memory, they are usually just integers. The only time - // they should be pointing to memory is when they are subslices of nonzero - // slices - let (alloc, offset, n, ty) = match value.ty.kind { - ty::Array(t, n) => { - match value.val { - ConstValue::ByRef { offset, alloc, .. } => ( - alloc, - offset, - n.eval_usize(cx.tcx, cx.param_env), - t, - ), - _ => span_bug!( - pat.span, - "array pattern is {:?}", value, - ), - } - }, - ty::Slice(t) => { - match value.val { - ConstValue::Slice { data, start, end } => ( - data, - Size::from_bytes(start as u64), - (end - start) as u64, - t, - ), - ConstValue::ByRef { .. } => { - // FIXME(oli-obk): implement `deref` for `ConstValue` - return None; - }, - _ => span_bug!( - pat.span, - "slice pattern constant must be scalar pair but is {:?}", - value, - ), - } + PatKind::Constant { value } if constructor.is_slice() => { + // We extract an `Option` for the pointer because slices of zero + // elements don't necessarily point to memory, they are usually + // just integers. The only time they should be pointing to memory + // is when they are subslices of nonzero slices. + let (alloc, offset, n, ty) = match value.ty.kind { + ty::Array(t, n) => { + match value.val { + ConstValue::ByRef { offset, alloc, .. } => ( + alloc, + offset, + n.eval_usize(cx.tcx, cx.param_env), + t, + ), + _ => span_bug!( + pat.span, + "array pattern is {:?}", value, + ), + } + }, + ty::Slice(t) => { + match value.val { + ConstValue::Slice { data, start, end } => ( + data, + Size::from_bytes(start as u64), + (end - start) as u64, + t, + ), + ConstValue::ByRef { .. } => { + // FIXME(oli-obk): implement `deref` for `ConstValue` + return None; }, _ => span_bug!( pat.span, - "unexpected const-val {:?} with ctor {:?}", + "slice pattern constant must be scalar pair but is {:?}", value, - constructor, ), - }; - if wild_patterns.len() as u64 == n { - // convert a constant slice/array pattern to a list of patterns. - let layout = cx.tcx.layout_of(cx.param_env.and(ty)).ok()?; - let ptr = Pointer::new(AllocId(0), offset); - (0..n).map(|i| { - let ptr = ptr.offset(layout.size * i, &cx.tcx).ok()?; - let scalar = alloc.read_scalar( - &cx.tcx, ptr, layout.size, - ).ok()?; - let scalar = scalar.not_undef().ok()?; - let value = ty::Const::from_scalar(cx.tcx, scalar, ty); - let pattern = Pat { - ty, - span: pat.span, - kind: box PatKind::Constant { value }, - }; - Some(&*cx.pattern_arena.alloc(pattern)) - }).collect() - } else { - None } - } - _ => { - // If the constructor is a: - // Single value: add a row if the constructor equals the pattern. - // Range: add a row if the constructor contains the pattern. - constructor_intersects_pattern(cx.tcx, cx.param_env, constructor, pat) - } + }, + _ => span_bug!( + pat.span, + "unexpected const-val {:?} with ctor {:?}", + value, + constructor, + ), + }; + if wild_patterns.len() as u64 == n { + // convert a constant slice/array pattern to a list of patterns. + let layout = cx.tcx.layout_of(cx.param_env.and(ty)).ok()?; + let ptr = Pointer::new(AllocId(0), offset); + (0..n).map(|i| { + let ptr = ptr.offset(layout.size * i, &cx.tcx).ok()?; + let scalar = alloc.read_scalar( + &cx.tcx, ptr, layout.size, + ).ok()?; + let scalar = scalar.not_undef().ok()?; + let value = ty::Const::from_scalar(cx.tcx, scalar, ty); + let pattern = Pat { + ty, + span: pat.span, + kind: box PatKind::Constant { value }, + }; + Some(&*cx.pattern_arena.alloc(pattern)) + }).collect() + } else { + None } } + PatKind::Constant { .. } | PatKind::Range { .. } => { // If the constructor is a: // Single value: add a row if the pattern contains the constructor. From 62ea39a1c95037dbdbfb49dc1b89d1b67562fddf Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 4 Oct 2019 11:04:14 +1000 Subject: [PATCH 2/5] Inline and remove `constructor_intersects_pattern()`. This is a 2% instruction count win on `unicode_normalization-check-clean`. --- src/librustc_mir/hair/pattern/_match.rs | 59 +++++++++++-------------- 1 file changed, 26 insertions(+), 33 deletions(-) diff --git a/src/librustc_mir/hair/pattern/_match.rs b/src/librustc_mir/hair/pattern/_match.rs index cf8ef8c769985..6d33c23dfdeb8 100644 --- a/src/librustc_mir/hair/pattern/_match.rs +++ b/src/librustc_mir/hair/pattern/_match.rs @@ -1625,36 +1625,6 @@ fn split_grouped_constructors<'p, 'tcx>( split_ctors } -/// Checks whether there exists any shared value in either `ctor` or `pat` by intersecting them. -fn constructor_intersects_pattern<'p, 'tcx>( - tcx: TyCtxt<'tcx>, - param_env: ty::ParamEnv<'tcx>, - ctor: &Constructor<'tcx>, - pat: &'p Pat<'tcx>, -) -> Option; 2]>> { - if should_treat_range_exhaustively(tcx, ctor) { - match (IntRange::from_ctor(tcx, param_env, ctor), IntRange::from_pat(tcx, param_env, pat)) { - (Some(ctor), Some(pat)) => { - ctor.intersection(&pat).map(|_| { - let (pat_lo, pat_hi) = pat.range.into_inner(); - let (ctor_lo, ctor_hi) = ctor.range.into_inner(); - assert!(pat_lo <= ctor_lo && ctor_hi <= pat_hi); - smallvec![] - }) - } - _ => None, - } - } else { - // Fallback for non-ranges and ranges that involve floating-point numbers, which are not - // conveniently handled by `IntRange`. For these cases, the constructor may not be a range - // so intersection actually devolves into being covered by the pattern. - match constructor_covered_by_range(tcx, param_env, ctor, pat) { - Ok(true) => Some(smallvec![]), - Ok(false) | Err(ErrorReported) => None, - } - } -} - fn constructor_covered_by_range<'tcx>( tcx: TyCtxt<'tcx>, param_env: ty::ParamEnv<'tcx>, @@ -1845,9 +1815,32 @@ fn specialize<'p, 'a: 'p, 'tcx>( PatKind::Constant { .. } | PatKind::Range { .. } => { // If the constructor is a: - // Single value: add a row if the pattern contains the constructor. - // Range: add a row if the constructor intersects the pattern. - constructor_intersects_pattern(cx.tcx, cx.param_env, constructor, pat) + // - Single value: add a row if the pattern contains the constructor. + // - Range: add a row if the constructor intersects the pattern. + if should_treat_range_exhaustively(cx.tcx, constructor) { + match (IntRange::from_ctor(cx.tcx, cx.param_env, constructor), + IntRange::from_pat(cx.tcx, cx.param_env, pat)) { + (Some(ctor), Some(pat)) => { + ctor.intersection(&pat).map(|_| { + let (pat_lo, pat_hi) = pat.range.into_inner(); + let (ctor_lo, ctor_hi) = ctor.range.into_inner(); + assert!(pat_lo <= ctor_lo && ctor_hi <= pat_hi); + smallvec![] + }) + } + _ => None, + } + } else { + // Fallback for non-ranges and ranges that involve + // floating-point numbers, which are not conveniently handled + // by `IntRange`. For these cases, the constructor may not be a + // range so intersection actually devolves into being covered + // by the pattern. + match constructor_covered_by_range(cx.tcx, cx.param_env, constructor, pat) { + Ok(true) => Some(smallvec![]), + Ok(false) | Err(ErrorReported) => None, + } + } } PatKind::Array { ref prefix, ref slice, ref suffix } | From a69e0e0ab402591e9e42c596054ea45df702f3e0 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 4 Oct 2019 13:16:37 +1000 Subject: [PATCH 3/5] Rearrange `IntRange::from_{ctor,pat}()`. This commit moves a lot of code around but doesn't change functionality at all. The main goal was for `from_pat()` to no longer call `from_ctor()`. The increase in inlining resulted in less function call overhead, for a 3% instruction count win on `unicode_normalization-check-clean`. --- src/librustc_mir/hair/pattern/_match.rs | 109 +++++++++++++++--------- 1 file changed, 69 insertions(+), 40 deletions(-) diff --git a/src/librustc_mir/hair/pattern/_match.rs b/src/librustc_mir/hair/pattern/_match.rs index 6d33c23dfdeb8..2a7be75650f29 100644 --- a/src/librustc_mir/hair/pattern/_match.rs +++ b/src/librustc_mir/hair/pattern/_match.rs @@ -834,6 +834,59 @@ struct IntRange<'tcx> { } impl<'tcx> IntRange<'tcx> { + #[inline] + fn is_integral(ty: Ty<'_>) -> bool { + match ty.kind { + ty::Char | ty::Int(_) | ty::Uint(_) => true, + _ => false, + } + } + + #[inline] + fn from_const( + tcx: TyCtxt<'tcx>, + param_env: ty::ParamEnv<'tcx>, + value: &Const<'tcx>, + ) -> Option> { + if Self::is_integral(value.ty) { + let ty = value.ty; + if let Some(val) = value.try_eval_bits(tcx, param_env, ty) { + let bias = IntRange::signed_bias(tcx, ty); + let val = val ^ bias; + Some(IntRange { range: val..=val, ty }) + } else { + None + } + } else { + None + } + } + + #[inline] + fn from_range( + tcx: TyCtxt<'tcx>, + lo: u128, + hi: u128, + ty: Ty<'tcx>, + end: &RangeEnd, + ) -> Option> { + if Self::is_integral(ty) { + // Perform a shift if the underlying types are signed, + // which makes the interval arithmetic simpler. + let bias = IntRange::signed_bias(tcx, ty); + let (lo, hi) = (lo ^ bias, hi ^ bias); + // Make sure the interval is well-formed. + if lo > hi || lo == hi && *end == RangeEnd::Excluded { + None + } else { + let offset = (*end == RangeEnd::Excluded) as u128; + Some(IntRange { range: lo..=(hi - offset), ty }) + } + } else { + None + } + } + fn from_ctor( tcx: TyCtxt<'tcx>, param_env: ty::ParamEnv<'tcx>, @@ -841,37 +894,9 @@ impl<'tcx> IntRange<'tcx> { ) -> Option> { // Floating-point ranges are permitted and we don't want // to consider them when constructing integer ranges. - fn is_integral(ty: Ty<'_>) -> bool { - match ty.kind { - ty::Char | ty::Int(_) | ty::Uint(_) => true, - _ => false, - } - } - match ctor { - ConstantRange(lo, hi, ty, end) if is_integral(ty) => { - // Perform a shift if the underlying types are signed, - // which makes the interval arithmetic simpler. - let bias = IntRange::signed_bias(tcx, ty); - let (lo, hi) = (lo ^ bias, hi ^ bias); - // Make sure the interval is well-formed. - if lo > hi || lo == hi && *end == RangeEnd::Excluded { - None - } else { - let offset = (*end == RangeEnd::Excluded) as u128; - Some(IntRange { range: lo..=(hi - offset), ty }) - } - } - ConstantValue(val) if is_integral(val.ty) => { - let ty = val.ty; - if let Some(val) = val.try_eval_bits(tcx, param_env, ty) { - let bias = IntRange::signed_bias(tcx, ty); - let val = val ^ bias; - Some(IntRange { range: val..=val, ty }) - } else { - None - } - } + ConstantRange(lo, hi, ty, end) => Self::from_range(tcx, *lo, *hi, ty, end), + ConstantValue(val) => Self::from_const(tcx, param_env, val), _ => None, } } @@ -881,22 +906,26 @@ impl<'tcx> IntRange<'tcx> { param_env: ty::ParamEnv<'tcx>, mut pat: &Pat<'tcx>, ) -> Option> { - let range = loop { + loop { match pat.kind { - box PatKind::Constant { value } => break ConstantValue(value), - box PatKind::Range(PatRange { lo, hi, end }) => break ConstantRange( - lo.eval_bits(tcx, param_env, lo.ty), - hi.eval_bits(tcx, param_env, hi.ty), - lo.ty, - end, - ), + box PatKind::Constant { value } => { + return Self::from_const(tcx, param_env, value); + } + box PatKind::Range(PatRange { lo, hi, end }) => { + return Self::from_range( + tcx, + lo.eval_bits(tcx, param_env, lo.ty), + hi.eval_bits(tcx, param_env, hi.ty), + &lo.ty, + &end, + ); + } box PatKind::AscribeUserType { ref subpattern, .. } => { pat = subpattern; }, _ => return None, } - }; - Self::from_ctor(tcx, param_env, &range) + } } // The return value of `signed_bias` should be XORed with an endpoint to encode/decode it. From 5515a976464e488949a6a7324ac41ecaf3246a19 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 4 Oct 2019 14:29:20 +1000 Subject: [PATCH 4/5] Introduce a special case in `IntRange::from_const`. The `if let Some(val) = value.try_eval_bits(...)` branch in `from_const()` is very hot for the `unicode_normalization` benchmark. This commit introduces a special-case alternative for scalars that avoids `try_eval_bits()` and all the functions it calls (`Const::eval()`, `ConstValue::try_to_bits()`, `ConstValue::try_to_scalar()`, and `Scalar::to_bits()`), instead extracting the result immediately. The type and value checking done by `Scalar::to_bits()` is replicated by moving it into a new function `Scalar::check_raw()` and using that new function in the special case. PR #64673 introduced some special-case handling of scalar types in `Const::try_eval_bits()`. This handling is now moved out of that function into the new `IntRange::integral_size_and_signed_bias` function. This commit reduces the instruction count for `unicode_normalization-check-clean` by about 10%. --- src/librustc/mir/interpret/value.rs | 11 +++++--- src/librustc/ty/sty.rs | 18 ++----------- src/librustc_mir/hair/pattern/_match.rs | 35 ++++++++++++++++++++----- 3 files changed, 38 insertions(+), 26 deletions(-) diff --git a/src/librustc/mir/interpret/value.rs b/src/librustc/mir/interpret/value.rs index 32f45cd9d4720..bbf00cc23ae88 100644 --- a/src/librustc/mir/interpret/value.rs +++ b/src/librustc/mir/interpret/value.rs @@ -343,14 +343,19 @@ impl<'tcx, Tag> Scalar { } } + #[inline(always)] + pub fn check_raw(data: u128, size: u8, target_size: Size) { + assert_eq!(target_size.bytes(), size as u64); + assert_ne!(size, 0, "you should never look at the bits of a ZST"); + Scalar::check_data(data, size); + } + /// Do not call this method! Use either `assert_bits` or `force_bits`. #[inline] pub fn to_bits(self, target_size: Size) -> InterpResult<'tcx, u128> { match self { Scalar::Raw { data, size } => { - assert_eq!(target_size.bytes(), size as u64); - assert_ne!(size, 0, "you should never look at the bits of a ZST"); - Scalar::check_data(data, size); + Self::check_raw(data, size, target_size); Ok(data) } Scalar::Ptr(_) => throw_unsup!(ReadPointerAsBytes), diff --git a/src/librustc/ty/sty.rs b/src/librustc/ty/sty.rs index e105c44d09ae5..d5bb18a2e3e2a 100644 --- a/src/librustc/ty/sty.rs +++ b/src/librustc/ty/sty.rs @@ -13,7 +13,7 @@ use rustc_macros::HashStable; use crate::ty::subst::{InternalSubsts, Subst, SubstsRef, GenericArg, GenericArgKind}; use crate::ty::{self, AdtDef, Discr, DefIdTree, TypeFlags, Ty, TyCtxt, TypeFoldable}; use crate::ty::{List, TyS, ParamEnvAnd, ParamEnv}; -use crate::ty::layout::{Size, Integer, IntegerExt, VariantIdx}; +use crate::ty::layout::VariantIdx; use crate::util::captures::Captures; use crate::mir::interpret::{Scalar, GlobalId}; @@ -24,7 +24,6 @@ use std::marker::PhantomData; use std::ops::Range; use rustc_target::spec::abi; use syntax::ast::{self, Ident}; -use syntax::attr::{SignedInt, UnsignedInt}; use syntax::symbol::{kw, InternedString}; use self::InferTy::*; @@ -2299,20 +2298,7 @@ impl<'tcx> Const<'tcx> { ty: Ty<'tcx>, ) -> Option { assert_eq!(self.ty, ty); - // This is purely an optimization -- layout_of is a pretty expensive operation, - // but if we can determine the size without calling it, we don't need all that complexity - // (hashing, caching, etc.). As such, try to skip it. - let size = match ty.kind { - ty::Bool => Size::from_bytes(1), - ty::Char => Size::from_bytes(4), - ty::Int(ity) => { - Integer::from_attr(&tcx, SignedInt(ity)).size() - } - ty::Uint(uty) => { - Integer::from_attr(&tcx, UnsignedInt(uty)).size() - } - _ => tcx.layout_of(param_env.with_reveal_all().and(ty)).ok()?.size, - }; + let size = tcx.layout_of(param_env.with_reveal_all().and(ty)).ok()?.size; // if `ty` does not depend on generic parameters, use an empty param_env self.eval(tcx, param_env).val.try_to_bits(size) } diff --git a/src/librustc_mir/hair/pattern/_match.rs b/src/librustc_mir/hair/pattern/_match.rs index 2a7be75650f29..c4f59e649b3ae 100644 --- a/src/librustc_mir/hair/pattern/_match.rs +++ b/src/librustc_mir/hair/pattern/_match.rs @@ -842,21 +842,42 @@ impl<'tcx> IntRange<'tcx> { } } + #[inline] + fn integral_size_and_signed_bias(tcx: TyCtxt<'tcx>, ty: Ty<'_>) -> Option<(Size, u128)> { + match ty.kind { + ty::Char => Some((Size::from_bytes(4), 0)), + ty::Int(ity) => { + let size = Integer::from_attr(&tcx, SignedInt(ity)).size(); + Some((size, 1u128 << (size.bits() as u128 - 1))) + } + ty::Uint(uty) => Some((Integer::from_attr(&tcx, UnsignedInt(uty)).size(), 0)), + _ => None, + } + } + #[inline] fn from_const( tcx: TyCtxt<'tcx>, param_env: ty::ParamEnv<'tcx>, value: &Const<'tcx>, ) -> Option> { - if Self::is_integral(value.ty) { + if let Some((target_size, bias)) = Self::integral_size_and_signed_bias(tcx, value.ty) { let ty = value.ty; - if let Some(val) = value.try_eval_bits(tcx, param_env, ty) { - let bias = IntRange::signed_bias(tcx, ty); - let val = val ^ bias; - Some(IntRange { range: val..=val, ty }) + let val = if let ConstValue::Scalar(Scalar::Raw { data, size }) = value.val { + // For this specific pattern we can skip a lot of effort and go + // straight to the result, after doing a bit of checking. (We + // could remove this branch and just use the next branch, which + // is more general but much slower.) + Scalar::<()>::check_raw(data, size, target_size); + data + } else if let Some(val) = value.try_eval_bits(tcx, param_env, ty) { + // This is a more general form of the previous branch. + val } else { - None - } + return None + }; + let val = val ^ bias; + Some(IntRange { range: val..=val, ty }) } else { None } From 2a3a5447418cc8e7a8b36ae4c9bdf8798a20b873 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 4 Oct 2019 16:22:13 +1000 Subject: [PATCH 5/5] Replace `flat_map()` with `filter_map()` in `is_useful_specialized()`. `filter_map()` is less general, but more efficient, and has the same effect in this case. This commit reduces the instruction count for `unicode_normalization-check-clean` by about 2%. --- src/librustc_mir/hair/pattern/_match.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/hair/pattern/_match.rs b/src/librustc_mir/hair/pattern/_match.rs index c4f59e649b3ae..3ea5805287724 100644 --- a/src/librustc_mir/hair/pattern/_match.rs +++ b/src/librustc_mir/hair/pattern/_match.rs @@ -1355,9 +1355,11 @@ fn is_useful_specialized<'p, 'a, 'tcx>( } }).collect(); let wild_patterns: Vec<_> = wild_patterns_owned.iter().collect(); - let matrix = Matrix(m.iter().flat_map(|r| { - specialize(cx, &r, &ctor, &wild_patterns) - }).collect()); + let matrix = Matrix( + m.iter() + .filter_map(|r| specialize(cx, &r, &ctor, &wild_patterns)) + .collect() + ); match specialize(cx, v, &ctor, &wild_patterns) { Some(v) => match is_useful(cx, &matrix, &v, witness) { UsefulWithWitness(witnesses) => UsefulWithWitness(