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.

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

Unify intrinsics body handling in StableMIR #126361

Merged
merged 1 commit into from
Jun 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 5 additions & 11 deletions compiler/rustc_smir/src/rustc_smir/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,10 @@ impl<'tcx> Context for TablesWrapper<'tcx> {
}

fn has_body(&self, def: DefId) -> bool {
let tables = self.0.borrow();
let def_id = tables[def];
tables.tcx.is_mir_available(def_id)
let mut tables = self.0.borrow_mut();
let tcx = tables.tcx;
let def_id = def.internal(&mut *tables, tcx);
tables.item_has_body(def_id)
}

fn foreign_modules(&self, crate_num: CrateNum) -> Vec<stable_mir::ty::ForeignModuleDef> {
Expand Down Expand Up @@ -322,13 +323,6 @@ impl<'tcx> Context for TablesWrapper<'tcx> {
tcx.intrinsic(def_id).unwrap().name.to_string()
}

fn intrinsic_must_be_overridden(&self, def: IntrinsicDef) -> bool {
let mut tables = self.0.borrow_mut();
let tcx = tables.tcx;
let def_id = def.0.internal(&mut *tables, tcx);
tcx.intrinsic_raw(def_id).unwrap().must_be_overridden
}

fn closure_sig(&self, args: &GenericArgs) -> PolyFnSig {
let mut tables = self.0.borrow_mut();
let tcx = tables.tcx;
Expand Down Expand Up @@ -515,7 +509,7 @@ impl<'tcx> Context for TablesWrapper<'tcx> {
let mut tables = self.0.borrow_mut();
let instance = tables.instances[def];
tables
.has_body(instance)
.instance_has_body(instance)
.then(|| BodyBuilder::new(tables.tcx, instance).build(&mut *tables))
}

Expand Down
21 changes: 19 additions & 2 deletions compiler/rustc_smir/src/rustc_smir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,33 @@ impl<'tcx> Tables<'tcx> {
self.mir_consts.create_or_fetch(constant)
}

pub(crate) fn has_body(&self, instance: Instance<'tcx>) -> bool {
/// Return whether the instance as a body available.
///
/// Items and intrinsics may have a body available from its definition.
/// Shims body may be generated depending on their type.
pub(crate) fn instance_has_body(&self, instance: Instance<'tcx>) -> bool {
let def_id = instance.def_id();
self.tcx.is_mir_available(def_id)
self.item_has_body(def_id)
|| !matches!(
instance.def,
ty::InstanceDef::Virtual(..)
| ty::InstanceDef::Intrinsic(..)
| ty::InstanceDef::Item(..)
)
}

/// Return whether the item has a body defined by the user.
///
/// Note that intrinsics may have a placeholder body that shouldn't be used in practice.
/// In StableMIR, we handle this case as if the body is not available.
pub(crate) fn item_has_body(&self, def_id: DefId) -> bool {
let must_override = if let Some(intrinsic) = self.tcx.intrinsic(def_id) {
intrinsic.must_be_overridden
} else {
false
};
!must_override && self.tcx.is_mir_available(def_id)
Comment on lines +74 to +79
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let must_override = if let Some(intrinsic) = self.tcx.intrinsic(def_id) {
intrinsic.must_be_overridden
} else {
false
};
!must_override && self.tcx.is_mir_available(def_id)
if let Some(intrinsic) = self.tcx.intrinsic(def_id) {
!intrinsic.must_be_overridden
} else {
self.tcx.is_mir_available(def_id)
}

the compiler guarantees than any !must_be_overridden intrinsic has a body

Copy link
Contributor Author

@celinval celinval Jun 13, 2024

Choose a reason for hiding this comment

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

That doesn't work though. The test check_intrinsic will ICE because size_of_val doesn't have a body and must_be_overriden is false.

thread 'rustc' panicked at compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs:208:1:
DefId(2:1632 ~ core[8aa9]::intrinsics::{extern#1}::size_of_val) does not have a "optimized_mir"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

ah right, we should probably automatically set that flag for bodyless intrinsics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if you want me to

}
}

/// Build a stable mir crate from a given crate number.
Expand Down
4 changes: 0 additions & 4 deletions compiler/stable_mir/src/compiler_interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,6 @@ pub trait Context {
/// Retrieve the plain function name of an intrinsic.
fn intrinsic_name(&self, def: IntrinsicDef) -> Symbol;

/// Returns whether the intrinsic has no meaningful body and all backends
/// need to shim all calls to it.
fn intrinsic_must_be_overridden(&self, def: IntrinsicDef) -> bool;

/// Retrieve the closure signature for the given generic arguments.
fn closure_sig(&self, args: &GenericArgs) -> PolyFnSig;

Expand Down
7 changes: 6 additions & 1 deletion compiler/stable_mir/src/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -658,6 +658,11 @@ impl FnDef {
with(|ctx| ctx.has_body(self.0).then(|| ctx.mir_body(self.0)))
}

// Check if the function body is available.
pub fn has_body(&self) -> bool {
with(|ctx| ctx.has_body(self.0))
}

/// Get the information of the intrinsic if this function is a definition of one.
pub fn as_intrinsic(&self) -> Option<IntrinsicDef> {
with(|cx| cx.intrinsic(self.def_id()))
Expand All @@ -684,7 +689,7 @@ impl IntrinsicDef {
/// Returns whether the intrinsic has no meaningful body and all backends
/// need to shim all calls to it.
pub fn must_be_overridden(&self) -> bool {
with(|cx| cx.intrinsic_must_be_overridden(*self))
with(|cx| !cx.has_body(self.0))
}
}

Expand Down
19 changes: 12 additions & 7 deletions tests/ui-fulldeps/stable-mir/check_intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,16 +55,19 @@ fn test_intrinsics() -> ControlFlow<()> {
///
/// If by any chance this test breaks because you changed how an intrinsic is implemented, please
/// update the test to invoke a different intrinsic.
///
/// In StableMIR, we only expose intrinsic body if they are not marked with
/// `rustc_intrinsic_must_be_overridden`.
fn check_instance(instance: &Instance) {
assert_eq!(instance.kind, InstanceKind::Intrinsic);
let name = instance.intrinsic_name().unwrap();
if instance.has_body() {
let Some(body) = instance.body() else { unreachable!("Expected a body") };
assert!(!body.blocks.is_empty());
assert_matches!(name.as_str(), "likely" | "vtable_size");
assert_eq!(&name, "likely");
} else {
assert!(instance.body().is_none());
assert_eq!(&name, "size_of_val");
assert_matches!(name.as_str(), "size_of_val" | "vtable_size");
}
}

Expand All @@ -75,11 +78,13 @@ fn check_def(fn_def: FnDef) {

let name = intrinsic.fn_name();
match name.as_str() {
"likely" | "size_of_val" => {
"likely" => {
assert!(!intrinsic.must_be_overridden());
assert!(fn_def.has_body());
}
"vtable_size" => {
"vtable_size" | "size_of_val" => {
assert!(intrinsic.must_be_overridden());
assert!(!fn_def.has_body());
}
_ => unreachable!("Unexpected intrinsic: {}", name),
}
Expand All @@ -96,9 +101,9 @@ impl<'a> MirVisitor for CallsVisitor<'a> {
TerminatorKind::Call { func, .. } => {
let TyKind::RigidTy(RigidTy::FnDef(def, args)) =
func.ty(self.locals).unwrap().kind()
else {
return;
};
else {
return;
};
self.calls.push((def, args.clone()));
}
_ => {}
Expand Down
Loading