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

try to get rid of mir::Const::normalize #130990

Merged
merged 3 commits into from
Sep 29, 2024
Merged

try to get rid of mir::Const::normalize #130990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
921a5ef
try to get rid of mir::Const::normalize
RalfJung Sep 28, 2024
c55c4c9
tweak Const::identity_unevaluated name and docs
RalfJung Sep 28, 2024
7eedb68
adjust test
RalfJung Sep 29, 2024
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: 3 additions & 13 deletions compiler/rustc_middle/src/mir/consts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,9 @@ pub enum Const<'tcx> {
}

impl<'tcx> Const<'tcx> {
pub fn identity_unevaluated(
/// Creates an unevaluated const from a `DefId` for a const item.
/// The binders of the const item still need to be instantiated.
pub fn from_unevaluated(
tcx: TyCtxt<'tcx>,
def_id: DefId,
) -> ty::EarlyBinder<'tcx, Const<'tcx>> {
Expand Down Expand Up @@ -329,18 +331,6 @@ impl<'tcx> Const<'tcx> {
}
}

/// Normalizes the constant to a value or an error if possible.
#[inline]
pub fn normalize(self, tcx: TyCtxt<'tcx>, param_env: ty::ParamEnv<'tcx>) -> Self {
match self.eval(tcx, param_env, DUMMY_SP) {
Ok(val) => Self::Val(val, self.ty()),
Err(ErrorHandled::Reported(guar, _span)) => {
Self::Ty(Ty::new_error(tcx, guar.into()), ty::Const::new_error(tcx, guar.into()))
}
Err(ErrorHandled::TooGeneric(_span)) => self,
}
}

#[inline]
pub fn try_eval_scalar(
self,
Expand Down
18 changes: 6 additions & 12 deletions compiler/rustc_mir_build/src/thir/cx/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,23 +699,17 @@ impl<'tcx> Cx<'tcx> {
}
}
hir::InlineAsmOperand::Const { ref anon_const } => {
let value = mir::Const::identity_unevaluated(
tcx,
anon_const.def_id.to_def_id(),
)
.instantiate_identity()
Copy link
Member

@compiler-errors compiler-errors Sep 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This will break tests/ui/asm/const-error.rs, for the record.

I don't care, though, and I also think we should not be obligated to eagerly raise an error for that test, because it's inconsistent with other and only works b/c the const isn't TooGeneric.

//@ only-x86_64
//@ needs-asm-support

// Test to make sure that we emit const errors eagerly for inline asm

use std::arch::asm;

fn test<T>() {
    unsafe {
        asm!("/* {} */", const 1 / 0);
        //~^ ERROR evaluation of
    }
}

fn main() {}

Copy link
Member

Choose a reason for hiding this comment

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

c.f.

//@ only-x86_64
//@ needs-asm-support

// Test to make sure that we emit const errors eagerly for inline asm

use std::arch::asm;

fn test<T>() {
    unsafe {
        asm!("/* {} */", const 1 / std::mem::size_of::<T>());
    }
}

Which obviously only fails if we mono something like test::<Zst>().

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yeah I just found 82f23d5. ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't emit const errors eagerly for inline const blocks. So if inline asm behave like those (inheriting the generics from the surrounding function), IMO we should not error eagerly for them either.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cc @Amanieu

Copy link
Member

Choose a reason for hiding this comment

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

So right, this brings up a bigger question of "should we try to eagerly eval possibly unreachable but otherwise monomorphic consts"?

From a technical perspective, this seems like something that could be done by a MIR pass if we wanted to do so, though, which calls const_eval_resolve on all unevaluated mir::Const::Unevaluated and swallows any TooGeneric, rather than doing it ad-hoc and within mir build just for these consts.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do have such a pass for const items, but those are (currently) always monomorphic.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have adjusted the test, which should make this PR ready to land.

.normalize(tcx, self.param_env);
let value =
mir::Const::from_unevaluated(tcx, anon_const.def_id.to_def_id())
.instantiate_identity();
let span = tcx.def_span(anon_const.def_id);

InlineAsmOperand::Const { value, span }
}
hir::InlineAsmOperand::SymFn { ref anon_const } => {
let value = mir::Const::identity_unevaluated(
tcx,
anon_const.def_id.to_def_id(),
)
.instantiate_identity()
.normalize(tcx, self.param_env);
let value =
mir::Const::from_unevaluated(tcx, anon_const.def_id.to_def_id())
.instantiate_identity();
let span = tcx.def_span(anon_const.def_id);

InlineAsmOperand::SymFn { value, span }
Expand Down
4 changes: 1 addition & 3 deletions compiler/rustc_mir_transform/src/jump_threading.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,9 +516,7 @@ impl<'a, 'tcx> TOFinder<'a, 'tcx> {
// Avoid handling them, though this could be extended in the future.
return;
}
let Some(value) =
value.const_.normalize(self.tcx, self.param_env).try_to_scalar_int()
else {
let Some(value) = value.const_.try_eval_scalar_int(self.tcx, self.param_env) else {
return;
};
let conds = conditions.map(self.arena, |c| Condition {
Expand Down
6 changes: 4 additions & 2 deletions tests/ui/asm/const-error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
//@ only-x86_64
//@ needs-asm-support
//@ check-pass

// Test to make sure that we emit const errors eagerly for inline asm
// Test to make sure that we emit const errors late for inline asm,
// which is consistent with inline const blocks.

use std::arch::asm;

fn test<T>() {
unsafe {
// No error here, as this does not get monomorphized.
asm!("/* {} */", const 1 / 0);
//~^ ERROR evaluation of
}
}

Expand Down
9 changes: 0 additions & 9 deletions tests/ui/asm/const-error.stderr
View file
Open in desktop

This file was deleted.

Loading