Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid layout calculations in assert_bits to speed up match checking #57546

Closed
wants to merge 1 commit into from
Closed

Avoid layout calculations in assert_bits to speed up match checking #57546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/librustc/ty/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2277,7 +2277,7 @@ impl<'a, 'gcx, 'tcx> AdtDef {
match tcx.const_eval(param_env.and(cid)) {
Ok(val) => {
// FIXME: Find the right type and use it instead of `val.ty` here
if let Some(b) = val.assert_bits(tcx.global_tcx(), param_env.and(val.ty)) {
if let Some(b) = val.assert_bits(param_env.and(val.ty)) {
trace!("discriminants: {} ({:?})", b, repr_type);
Some(Discr {
val: b,
Expand Down
27 changes: 11 additions & 16 deletions src/librustc/ty/sty.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2152,20 +2152,19 @@ impl<'tcx> Const<'tcx> {
}

#[inline]
pub fn assert_bits(
&self,
tcx: TyCtxt<'_, '_, '_>,
ty: ParamEnvAnd<'tcx, Ty<'tcx>>,
) -> Option<u128> {
pub fn assert_bits(&self, ty: ParamEnvAnd<'tcx, Ty<'tcx>>) -> Option<u128> {
assert_eq!(self.ty, ty.value);
let ty = tcx.lift_to_global(&ty).unwrap();
let size = tcx.layout_of(ty).ok()?.size;
self.val.try_to_bits(size)
match self.val.try_to_scalar()? {
Scalar::Bits { bits, .. } => {
Some(bits)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fishy, we shouldn't usually return bits without checking that the size is correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that they should already be correct because the type of the constant is the same. Sanity checks would have bailed long before if the number of bits didn't match the type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are constants expected to have sizes that don't match their type's size? If so, the premise to this PR is broken. If not, it would seem more sensible to me to assert that the sizes match when the constant is created, rather than on each time we read from it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are constants expected to have sizes that don't match their type's size?

No, that's why there are assertions in place that check the size. If we happened to get the size wrong, that's a bug. These assertions just make sure that we don't screw up (as we have done before, this is not just hypothetical, it's easy to get wrong in some places, but less so nowadays).

If not, it would seem more sensible to me to assert that the sizes match when the constant is created, rather than on each time we read from it.

I believe that we are doing this now, not directly when creating the ty::Const, but during const_eval.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I am not very happy about removing a sanity check that costs no measurable performance and caught real bugs. So I'd r-.

But if @oli-obk vetoes me on this, that's fine for me. Just please add a comment stating very explicitly that we are deliberately omitting a sanity check here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right, the assertion would have triggered then anyway. I've just been confused, because @RalfJung's comment sounded to me as if the type equality assertion is not enough, and the sizes could be mismatched regardless.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the type equality assertion is not enough, and the sizes could be mismatched regardless.

It is not enough and there could be a mismatch, pointing to a bug elsewhere. I am not sure if all Const constructors have this check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively we can completely move the sanity check to debug assertions. Some #[inline] annotations in appropriate places should make release mode optimize away the leftover arguments

}
Scalar::Ptr(_) => None,
}
}

#[inline]
pub fn assert_bool(&self, tcx: TyCtxt<'_, '_, '_>) -> Option<bool> {
self.assert_bits(tcx, ParamEnv::empty().and(tcx.types.bool)).and_then(|v| match v {
self.assert_bits(ParamEnv::empty().and(tcx.types.bool)).and_then(|v| match v {
0 => Some(false),
1 => Some(true),
_ => None,
Expand All @@ -2174,16 +2173,12 @@ impl<'tcx> Const<'tcx> {

#[inline]
pub fn assert_usize(&self, tcx: TyCtxt<'_, '_, '_>) -> Option<u64> {
self.assert_bits(tcx, ParamEnv::empty().and(tcx.types.usize)).map(|v| v as u64)
self.assert_bits(ParamEnv::empty().and(tcx.types.usize)).map(|v| v as u64)
}

#[inline]
pub fn unwrap_bits(
&self,
tcx: TyCtxt<'_, '_, '_>,
ty: ParamEnvAnd<'tcx, Ty<'tcx>>,
) -> u128 {
self.assert_bits(tcx, ty).unwrap_or_else(||
pub fn unwrap_bits(&self, ty: ParamEnvAnd<'tcx, Ty<'tcx>>) -> u128 {
self.assert_bits(ty).unwrap_or_else(||
bug!("expected bits of {}, got {:#?}", ty.value, self))
}

Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/build/matches/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
let switch_ty = ty::ParamEnv::empty().and(switch_ty);
indices.entry(value)
.or_insert_with(|| {
options.push(value.unwrap_bits(self.hir.tcx(), switch_ty));
options.push(value.unwrap_bits(switch_ty));
options.len() - 1
});
true
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_mir/hair/pattern/_match.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -844,7 +844,7 @@ impl<'tcx> IntRange<'tcx> {
}
ConstantValue(val) if is_integral(val.ty) => {
let ty = val.ty;
if let Some(val) = val.assert_bits(tcx, ty::ParamEnv::empty().and(ty)) {
if let Some(val) = val.assert_bits(ty::ParamEnv::empty().and(ty)) {
let bias = IntRange::signed_bias(tcx, ty);
let val = val ^ bias;
Some(IntRange { range: val..=val, ty })
Expand Down Expand Up @@ -1458,7 +1458,7 @@ fn slice_pat_covered_by_const<'tcx>(
{
match pat.kind {
box PatternKind::Constant { value } => {
let b = value.unwrap_bits(tcx, ty::ParamEnv::empty().and(pat.ty));
let b = value.unwrap_bits(ty::ParamEnv::empty().and(pat.ty));
assert_eq!(b as u8 as u128, b);
if b as u8 != *ch {
return Ok(false);
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/transform/simplify_branches.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ impl MirPass for SimplifyBranches {
discr: Operand::Constant(ref c), switch_ty, ref values, ref targets, ..
} => {
let switch_ty = ParamEnv::empty().and(switch_ty);
let constant = c.literal.map_evaluated(|c| c.assert_bits(tcx, switch_ty));
let constant = c.literal.map_evaluated(|c| c.assert_bits(switch_ty));
if let Some(constant) = constant {
let (otherwise, targets) = targets.split_last().unwrap();
let mut ret = TerminatorKind::Goto { target: *otherwise };
Expand Down