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

Merge Promoted and Static variants of mir::Place #53848

Closed
oli-obk opened this issue Aug 31, 2018 · 6 comments · Fixed by #59232
Closed

Merge Promoted and Static variants of mir::Place #53848

oli-obk opened this issue Aug 31, 2018 · 6 comments · Fixed by #59232
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Aug 31, 2018

This would allow merging most of the code in

Promoted(ref promoted) => {

I suggest to create a single variant (Maybe just leave the Static variant), but modify the Static struct fields for the DefId (update docs that the DefId is the one of the containing function in case of a promoted) and additionally add an Option<Promoted> field.

@oli-obk oli-obk added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. A-const-eval Area: Constant evaluation (MIR interpretation) S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Aug 31, 2018
@csmoe
Copy link
Member

csmoe commented Aug 31, 2018

I'm working on changing the representation of mir::Place, since that may take some time to be merged, so block this for easing rebase-stuff at first and this issue will be unblocked as soon as the PR merged.

@wesleywiser
Copy link
Member

I'm interested on working on this when @csmoe's changes land.

@saleemjaffer
Copy link
Contributor

@oli-obk I would like to take this up.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 13, 2019

Cool! So I think the changes necessary are

  1. Remove the Promoted variant
    Promoted(Box<(Promoted, Ty<'tcx>)>),
  2. Add an Option<Promoted> field to https://github.com/rust-lang/rust/blob/master/src/librustc/mir/mod.rs#L1920-L1923
  3. Recompile and follow the fallout to always create a Static variant
    • Everywhere where we used to create Promoted, fill in the Option<Promoted> with Some
    • Everywhere where we used to create Static, use None
  4. Merge the two arms in
    Base(PlaceBase::Promoted(ref promoted)) => {
    (this might be a little finicky, so maybe start out by just doing what the current code is doing by differentiating between the Some and the None case)

@saleemjaffer
Copy link
Contributor

@oli-obk

fn promote_candidate(mut self, candidate: Candidate) {
let mut operand = {
let promoted = &mut self.promoted;
let promoted_id = Promoted::new(self.source.promoted.len());
let mut promoted_place = |ty, span| {
promoted.span = span;
promoted.local_decls[RETURN_PLACE] =
LocalDecl::new_return_place(ty, span);
Place::Base(PlaceBase::Promoted(box (promoted_id, ty)))
};

If i have to replace the Place::Base(PlaceBase::Promoted(box (promoted_id, ty))) with the PlaceBase::Static variant, what do I do for the def_id for PlaceBase::Static?

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 16, 2019

the DefId should be the one of the current function. So for now you'll need to give

struct Promoter<'a, 'tcx: 'a> {
a DefId field that refers to the current function. Then you can access the field here

Centril added a commit to Centril/rust that referenced this issue Mar 26, 2019
…oli-obk

Merge `Promoted` and `Static` in `mir::Place`

fixes rust-lang#53848
Centril added a commit to Centril/rust that referenced this issue Mar 26, 2019
…oli-obk

Merge `Promoted` and `Static` in `mir::Place`

fixes rust-lang#53848
Centril added a commit to Centril/rust that referenced this issue Mar 26, 2019
…oli-obk

Merge `Promoted` and `Static` in `mir::Place`

fixes rust-lang#53848
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants