Skip to content

interpret: move nullary-op evaluation into operator.rs #128697

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

Merged
merged 1 commit into from
Aug 6, 2024
Merged

interpret: move nullary-op evaluation into operator.rs #128697

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 compiler/rustc_const_eval/src/const_eval/machine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
_unwind: mir::UnwindAction,
) -> InterpResult<'tcx, Option<ty::Instance<'tcx>>> {
// Shared intrinsics.
if ecx.emulate_intrinsic(instance, args, dest, target)? {
if ecx.eval_intrinsic(instance, args, dest, target)? {
return Ok(None);
}
let intrinsic_name = ecx.tcx.item_name(instance.def_id());
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_const_eval/src/interpret/intrinsics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
/// Returns `true` if emulation happened.
/// Here we implement the intrinsics that are common to all Miri instances; individual machines can add their own
/// intrinsic handling.
pub fn emulate_intrinsic(
pub fn eval_intrinsic(
&mut self,
instance: ty::Instance<'tcx>,
args: &[OpTy<'tcx, M::Provenance>],
Expand Down Expand Up @@ -447,7 +447,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
Ok(true)
}

pub(super) fn emulate_nondiverging_intrinsic(
pub(super) fn eval_nondiverging_intrinsic(
&mut self,
intrinsic: &NonDivergingIntrinsic<'tcx>,
) -> InterpResult<'tcx> {
Expand Down
37 changes: 36 additions & 1 deletion compiler/rustc_const_eval/src/interpret/operator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
use either::Either;
use rustc_apfloat::{Float, FloatConvert};
use rustc_middle::mir::interpret::{InterpResult, Scalar};
use rustc_middle::mir::NullOp;
use rustc_middle::ty::layout::{LayoutOf, TyAndLayout};
use rustc_middle::ty::{self, FloatTy, ScalarInt};
use rustc_middle::ty::{self, FloatTy, ScalarInt, Ty};
use rustc_middle::{bug, mir, span_bug};
use rustc_span::symbol::sym;
use tracing::trace;
Expand Down Expand Up @@ -480,4 +481,38 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
}
}
}

pub fn nullary_op(
&self,
null_op: NullOp<'tcx>,
arg_ty: Ty<'tcx>,
) -> InterpResult<'tcx, ImmTy<'tcx, M::Provenance>> {
use rustc_middle::mir::NullOp::*;

let layout = self.layout_of(arg_ty)?;
let usize_layout = || self.layout_of(self.tcx.types.usize).unwrap();

Ok(match null_op {
SizeOf => {
if !layout.abi.is_sized() {
span_bug!(self.cur_span(), "unsized type for `NullaryOp::SizeOf`");
}
let val = layout.size.bytes();
ImmTy::from_uint(val, usize_layout())
}
AlignOf => {
if !layout.abi.is_sized() {
span_bug!(self.cur_span(), "unsized type for `NullaryOp::AlignOf`");
}
let val = layout.align.abi.bytes();
ImmTy::from_uint(val, usize_layout())
}
OffsetOf(fields) => {
let val =
self.tcx.offset_of_subfield(self.param_env, layout, fields.iter()).bytes();
ImmTy::from_uint(val, usize_layout())
}
UbChecks => ImmTy::from_bool(self.tcx.sess.ub_checks(), *self.tcx),
Copy link
Member Author

Choose a reason for hiding this comment

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

The type isn't even used here... this makes me wonder whether we should remove the type from the Rvalue::NullaryOp and instead have it in NullaryOp::SizeOf etc?

Cc @scottmcm

Copy link
Member

Choose a reason for hiding this comment

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

Sticking a type in NullaryOp probably complicates a bunch of places where they're lifetime-free and Copy, so I'm not enthusiastic about that.

TBH, if we're going to refactor stuff here, I'd love to just kill UnOp::SizeOf and UnOp::AlignOf entirely, and use associated consts on a magic trait, so it's just switchInt <T as Foo>::SIZE instead of needing pointless temporaries.

Since UbChecks is pretty weird -- it's a constant but it's not constant -- it might better to just split it out entirely since it works so differently from all the other UnOps...

Copy link
Member

Choose a reason for hiding this comment

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

I would like to see an attempt to reimplement UbChecks as a const... Somehow. It would make a lot more sense.

Copy link
Member Author

@RalfJung RalfJung Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sticking a type in NullaryOp probably complicates a bunch of places where they're lifetime-free and Copy, so I'm not enthusiastic about that.

NullaryOp already has a 'tcx lifetime and Ty<'tcx> is Copy, so it doesn't change anything in this regard.

Copy link
Member Author

@RalfJung RalfJung Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Since UbChecks is pretty weird -- it's a constant but it's not constant -- it might better to just split it out entirely since it works so differently from all the other UnOps...

I would like to see an attempt to reimplement UbChecks as a const... Somehow. It would make a lot more sense.

It's not a constant. It can have different values in different compilation units, that's its entire point. So I think a NullaryOp makes a lot more sense for it than trying to make it a constant.

})
}
}
43 changes: 8 additions & 35 deletions compiler/rustc_const_eval/src/interpret/step.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@

use either::Either;
use rustc_index::IndexSlice;
use rustc_middle::ty::layout::LayoutOf;
use rustc_middle::{bug, mir, span_bug};
use rustc_middle::{bug, mir};
use rustc_target::abi::{FieldIdx, FIRST_VARIANT};
use tracing::{info, instrument, trace};

Expand Down Expand Up @@ -94,7 +93,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
M::retag_place_contents(self, *kind, &dest)?;
}

Intrinsic(box intrinsic) => self.emulate_nondiverging_intrinsic(intrinsic)?,
Intrinsic(box intrinsic) => self.eval_nondiverging_intrinsic(intrinsic)?,

// Evaluate the place expression, without reading from it.
PlaceMention(box place) => {
Expand Down Expand Up @@ -179,6 +178,12 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
self.write_immediate(*result, &dest)?;
}

NullaryOp(null_op, ty) => {
let ty = self.instantiate_from_current_frame_and_normalize_erasing_regions(ty)?;
let val = self.nullary_op(null_op, ty)?;
self.write_immediate(*val, &dest)?;
}

Aggregate(box ref kind, ref operands) => {
self.write_aggregate(kind, operands, &dest)?;
}
Expand Down Expand Up @@ -230,38 +235,6 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
self.write_immediate(*val, &dest)?;
}

NullaryOp(ref null_op, ty) => {
let ty = self.instantiate_from_current_frame_and_normalize_erasing_regions(ty)?;
let layout = self.layout_of(ty)?;
if let mir::NullOp::SizeOf | mir::NullOp::AlignOf = null_op
&& layout.is_unsized()
{
span_bug!(
self.frame().current_span(),
"{null_op:?} MIR operator called for unsized type {ty}",
);
}
let val = match null_op {
mir::NullOp::SizeOf => {
let val = layout.size.bytes();
Scalar::from_target_usize(val, self)
}
mir::NullOp::AlignOf => {
let val = layout.align.abi.bytes();
Scalar::from_target_usize(val, self)
}
mir::NullOp::OffsetOf(fields) => {
let val = self
.tcx
.offset_of_subfield(self.param_env, layout, fields.iter())
.bytes();
Scalar::from_target_usize(val, self)
}
mir::NullOp::UbChecks => Scalar::from_bool(self.tcx.sess.ub_checks()),
};
self.write_scalar(val, &dest)?;
}

ShallowInitBox(ref operand, _) => {
let src = self.eval_operand(operand, None)?;
let v = self.read_immediate(&src)?;
Expand Down
2 changes: 1 addition & 1 deletion src/tools/miri/src/intrinsics/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let this = self.eval_context_mut();

// See if the core engine can handle this intrinsic.
if this.emulate_intrinsic(instance, args, dest, ret)? {
if this.eval_intrinsic(instance, args, dest, ret)? {
return Ok(None);
}
let intrinsic_name = this.tcx.item_name(instance.def_id());
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/const-generics/generic_const_exprs/issue-80742.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: internal compiler error: compiler/rustc_const_eval/src/interpret/step.rs:LL:CC: SizeOf MIR operator called for unsized type dyn Debug
error: internal compiler error: compiler/rustc_const_eval/src/interpret/operator.rs:LL:CC: unsized type for `NullaryOp::SizeOf`
--> $SRC_DIR/core/src/mem/mod.rs:LL:COL

Box<dyn Any>
Expand Down