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

Conversation

RalfJung
Copy link
Member

It was easy to make this compile, let's see if anything breaks...

r? @compiler-errors

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 28, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 28, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@@ -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.

@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 29, 2024

📌 Commit 7eedb68 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 29, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 29, 2024
…iaskrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#123932 (restate GlobalAlloc method safety preconditions in terms of what the caller has to do for greater clarity)
 - rust-lang#129003 (Improve Ord docs)
 - rust-lang#130972 (stabilize const_cell_into_inner)
 - rust-lang#130990 (try to get rid of mir::Const::normalize)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a0ae32d into rust-lang:master Sep 29, 2024
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 29, 2024
Rollup merge of rust-lang#130990 - RalfJung:mir-const-normalize, r=compiler-errors

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

It was easy to make this compile, let's see if anything breaks...

r? `@compiler-errors`
@rustbot rustbot added this to the 1.83.0 milestone Sep 29, 2024
@RalfJung RalfJung deleted the mir-const-normalize branch September 30, 2024 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants