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

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

Merged
merged 3 commits into from
Sep 29, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
12 changes: 0 additions & 12 deletions compiler/rustc_middle/src/mir/consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,18 +329,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
6 changes: 2 additions & 4 deletions compiler/rustc_mir_build/src/thir/cx/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -703,8 +703,7 @@ impl<'tcx> Cx<'tcx> {
tcx,
anon_const.def_id.to_def_id(),
)
.instantiate_identity()
Copy link
Member

@compiler-errors compiler-errors Sep 28, 2024

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.

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);
.instantiate_identity();
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
let span = tcx.def_span(anon_const.def_id);

InlineAsmOperand::Const { value, span }
Expand All @@ -714,8 +713,7 @@ impl<'tcx> Cx<'tcx> {
tcx,
anon_const.def_id.to_def_id(),
)
.instantiate_identity()
.normalize(tcx, self.param_env);
.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
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
Loading