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

codegen: be more explicit about setting giving names to allocas. #64408

Merged
merged 1 commit into from
Sep 14, 2019

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Sep 12, 2019

This is a follow-up to #64149 and a prerequisite for the codegen side of #56231.

Moving to set_var_name everywhere will allow better integration with MIR variable debuginfo, I suspect (we could block the PR on finding out, I don't care too much about landing this first).

In this PR I removed some miscellaneous hardcoded names (which is why I had to fix a test), but if we want to keep them I could use set_var_name (it could end up being confusing, though, if there's a Rust variable with the same name).

Something I noticed while doing this is that we give arguments:

r? @rkruppe

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 12, 2019
@hanna-kruppe
Copy link
Contributor

My gut feeling is that some of these hardcoded names are potentially helpful. But others seem like useless cruft and the benefit of the others is marginal, so I'm perfectly fine with losing them all for the sake of simplicity. FWIW if we ever reintroduce some names for non-source-level-things, the confusion with Rust variables could be avoided by a naming scheme that can't collide (e.g., prefixing with $).

Regarding naming other MIR locals, I don't care too much for arguments specifically but I could imagine it's occasionally useful to have LLVM IR names that indicate which MIR local (if any) an LLVM-level alloca or SSA value corresponds to. I don't think it's high priority though, and it should be done with care so that the artificial fallback name doesn't override the meaningful source-level name.

One thing I'm not entirely clear about is the future relation between set_var_name and debuginfo. I've been assuming set_var_name would grow into something that sets the value names and also emits debuginfo, and this change would be clearly helpful then. But there's a few places (such as those discussed above) where we might want to name an LLVM value without attaching debuginfo, since they have no real source level meaning. Is that going to be an issue?

In any case, this diff LGTM so @bors r+

@bors
Copy link
Contributor

bors commented Sep 13, 2019

📌 Commit e9214a1 has been approved by rkruppe

@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 13, 2019
@eddyb
Copy link
Member Author

eddyb commented Sep 13, 2019

But there's a few places (such as those discussed above) where we might want to name an LLVM value without attaching debuginfo, since they have no real source level meaning. Is that going to be an issue?

No, so far I don't couple them that tightly in any of my WIP branches/plans.
Instead, I have a helper with handles debuginfo + (LLVM IR) name for a given MIR local, and everything else can just call set_var_name directly if needed (I guess that function could be renamed).

@eddyb
Copy link
Member Author

eddyb commented Sep 13, 2019

@bors r- (included in #64435)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 13, 2019
Centril added a commit to Centril/rust that referenced this pull request Sep 13, 2019
codegen: use "_N" (like for other locals) instead of "argN", for argument names.

Based on rust-lang#64408 (second commit is new), fixing something I mentioned in rust-lang#64408 (which turned to be an immediate blocker for unifying relevant codepaths).

Closes rust-lang#64408 (by containing it).

r? @rkruppe
Centril added a commit to Centril/rust that referenced this pull request Sep 14, 2019
codegen: use "_N" (like for other locals) instead of "argN", for argument names.

Based on rust-lang#64408 (second commit is new), fixing something I mentioned in rust-lang#64408 (which turned to be an immediate blocker for unifying relevant codepaths).

Closes rust-lang#64408 (by containing it).

r? @rkruppe
Centril added a commit to Centril/rust that referenced this pull request Sep 14, 2019
codegen: use "_N" (like for other locals) instead of "argN", for argument names.

Based on rust-lang#64408 (second commit is new), fixing something I mentioned in rust-lang#64408 (which turned to be an immediate blocker for unifying relevant codepaths).

Closes rust-lang#64408 (by containing it).

r? @rkruppe
@bors bors closed this in bf1253b Sep 14, 2019
@bors bors merged commit e9214a1 into rust-lang:master Sep 14, 2019
@eddyb eddyb deleted the codegen-alloca-names branch September 14, 2019 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants