-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Conversation
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 In any case, this diff LGTM so @bors r+ |
📌 Commit e9214a1 has been approved by |
No, so far I don't couple them that tightly in any of my WIP branches/plans. |
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
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
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
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:
arg0
,arg1
, etc. LLVM IR names (leftover from early MIR days?!)_1
,_2
, etc. MIR local indices (EDIT: see codegen: use "_N" (like for other locals) instead of "argN", for argument names. #64435)_123
etc. as LLVM IR names for unnamed locals in general?r? @rkruppe