Skip to content

Make PlaceElem::Index take a Place. #106594

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

Closed
wants to merge 1 commit into from

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Jan 8, 2023

Fixes #106141

This makes the handling of Places much more general. I'm not completely sure this is a good idea.

@rustbot
Copy link
Collaborator

rustbot commented Jan 8, 2023

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

@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 Jan 8, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 8, 2023

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Jan 8, 2023

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@bjorn3
Copy link
Member

bjorn3 commented Jan 8, 2023

I believe we actually moved away from arbitrary nesting of places in the past.

@RalfJung
Copy link
Member

RalfJung commented Jan 8, 2023

Yeah, I think this is un-doing a simplification that happened a few years back.

Why are general places in index projections needed to fix #106141?

@JakobDegen
Copy link
Contributor

Yeah, I also feel pretty strongly that we should not do this. For example, this opens up a whole can of worms around evaluation order that I would greatly prefer we don't make any harder on ourselves

@oli-obk
Copy link
Contributor

oli-obk commented Jan 9, 2023

I would prefer to actually remove the index in favor of having an rvalue for it, similar to the ongoing work to removing the deref projection. Everything else has already been said. I'm going to go ahead and close this, as afaik no on wg-mir-opt wants to move in this direction.

@oli-obk oli-obk closed this Jan 9, 2023
@cjgillot cjgillot deleted the place-index branch January 9, 2023 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE in MIR inlining with -Zmir-opt-level=3
8 participants