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

Refactor non_ssa_locals to remove LocalAnalyzer::process_place #72931

Open
ecstatic-morse opened this issue Jun 2, 2020 · 1 comment
Open
Labels
A-codegen Area: Code generation C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Jun 2, 2020

While doing an initial implementation of rust-lang/compiler-team#300, I began replacing all uses of {Non,}MutatingUseContext::Projection. One user was rustc_codegen_ssa::mir::analyze::LocalAnalyzer, which is doing a custom recursive traversal in process_place. There's a HACK comment from @eddyb suggesting that this should be rewritten.

@eddyb, did you have something specific in mind? I spent some time trying to decipher the logic here, but there's parts I don't understand. For example, there's a long comment about why we need to call visit_local for NonUseContext::VarDebugInfo, but visit_local does nothing for NonUses. What's the indended behavior here?

Additionally, LocalAnalyzer visits basic blocks in numerical order, but this code assumes we will visit the assignment to a local before any uses of it. This isn't a soundness issue; The worst thing that can happen is that locals are placed onto the stack unnecessarily. However, I think we should be visiting basic blocks in RPO, no?

@ecstatic-morse ecstatic-morse added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-codegen Area: Code generation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 2, 2020
@eddyb
Copy link
Member

eddyb commented Jul 15, 2020

Ah, the visit_local call could've been removed after/thanks to #68961 (specifically, this change) - somehow I forgot to do that.

And, wow, I can't believe we made the RPO mistake. That line has been the same since the start (6a5b263).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
2 participants