Skip to content

codegen: create DIVariables ahead of using them with llvm.dbg.declare. #68665

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

Merged
merged 3 commits into from
Feb 3, 2020

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jan 30, 2020

Instead of having rustc_codegen_llvm bundle creation of a DIVariable and llvm.dbg.declare into a single operation, they are now two separate methods, and the DIVariable is created earlier, specifically when mir::VarDebugInfos are being partitioned into locals.

While this isn't currently needed, it's a prerequisite for #48300, which adds fragmented debuginfo, where one mir::VarDebugInfo has multiple parts of itself mapped to different mir::Places.
For debuggers to see one composite variable instead of several ones with the same name, we need to create a single DIVariable and share it between multiple llvm.dbg.declare calls, which are likely pointing to different MIR locals.
That makes the per_local_var_debug_info partitioning a good spot to do this in, as we can create exactly one DIVariable per mir::VarDebugInfo, and refer to it as many things as needed.

I'm opening this PR separately because I want to test its perf impact in isolation (see #48300 (comment)).

r? @nagisa or @oli-obk cc @michaelwoerister @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 30, 2020
@eddyb
Copy link
Member Author

eddyb commented Jan 30, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Jan 30, 2020

⌛ Trying commit f397140 with merge e90b4ff2541f55c702a0fc68a959ef37bc8722a2...

@bors
Copy link
Collaborator

bors commented Jan 30, 2020

☀️ Try build successful - checks-azure
Build commit: e90b4ff2541f55c702a0fc68a959ef37bc8722a2 (e90b4ff2541f55c702a0fc68a959ef37bc8722a2)

@nagisa
Copy link
Member

nagisa commented Jan 30, 2020

r=me on implementation, pending timing results.

@Mark-Simulacrum
Copy link
Member

@rust-timer build e90b4ff2541f55c702a0fc68a959ef37bc8722a2

@Mark-Simulacrum
Copy link
Member

@rust-timer build e90b4ff2541f55c702a0fc68a959ef37bc8722a2

I think I was just unlucky

@rust-timer
Copy link
Collaborator

Queued e90b4ff2541f55c702a0fc68a959ef37bc8722a2 with parent 3024c4e, future comparison URL.

@eddyb
Copy link
Member Author

eddyb commented Jan 31, 2020

@bjorn3 That looks like the same thing we saw on #48300 (comment).

The only difference between this PR and #48300 with the optimization of by default (which it was for the run I just linked) are some MIR data structure changes, so I think my hunch was right that it wasn't actually (entirely) noise in that perf run, since syn-opt and inflate-opt also show up here.

@bjorn3
Copy link
Member

bjorn3 commented Jan 31, 2020

@bjorn3 That looks like the same thing we saw on #48300 (comment).

test SROA PR this PR
syn clean 0.9% -3.1%
syn baseline incr -2.8% 1.3%
syn patched incr -4.8% 1.9%
-- -- --
inflate patched 1.5% -1.4%

(regressions marked bold)

This PR seems to have the opposite effect of the SROA PR. The slow get faster, while the fast get slower.

@eddyb
Copy link
Member Author

eddyb commented Jan 31, 2020

Actually, it should be impossible for this PR to have an effect for -opt runs because none of the code changed here even runs. So it must be noise in that case?

I just checked a hunch and we don't seem to be creating DIVariables that don't get used later, but I'm not sure how much of what I'm testing actually has -C debuginfo-level=2 (only debuginfo tests?).
EDIT: tried compiling something bigger and my check didn't trip there either.
So there's probably not more DIVariables after this PR, than before.

@eddyb eddyb force-pushed the debuginfo-early-create-var branch from f397140 to e35dfad Compare February 3, 2020 10:34
@eddyb
Copy link
Member Author

eddyb commented Feb 3, 2020

Assuming we're mostly seeing noise:
@bors r=nagisa

@bors
Copy link
Collaborator

bors commented Feb 3, 2020

📌 Commit e35dfad has been approved by nagisa

@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 Feb 3, 2020
@bors
Copy link
Collaborator

bors commented Feb 3, 2020

⌛ Testing commit e35dfad with merge bdd946d...

bors added a commit that referenced this pull request Feb 3, 2020
codegen: create DIVariables ahead of using them with llvm.dbg.declare.

Instead of having `rustc_codegen_llvm` bundle creation of a `DIVariable` and `llvm.dbg.declare` into a single operation, they are now two separate methods, and the `DIVariable` is created earlier, specifically when `mir::VarDebugInfo`s are being partitioned into locals.

While this isn't currently needed, it's a prerequisite for #48300, which adds fragmented debuginfo, where one `mir::VarDebugInfo` has multiple parts of itself mapped to different `mir::Place`s.
For debuggers to see one composite variable instead of several ones with the same name, we need to create a single `DIVariable` and share it between multiple `llvm.dbg.declare` calls, which are likely pointing to different MIR locals.
That makes the `per_local_var_debug_info` partitioning a good spot to do this in, as we can create *exactly* one `DIVariable` per `mir::VarDebugInfo`, and refer to it as many things as needed.

I'm opening this PR separately because I want to test its perf impact in isolation (see #48300 (comment)).

r? @nagisa or @oli-obk cc @michaelwoerister @nikomatsakis
@bors
Copy link
Collaborator

bors commented Feb 3, 2020

☀️ Test successful - checks-azure
Approved by: nagisa
Pushing bdd946d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 3, 2020
@bors bors merged commit e35dfad into rust-lang:master Feb 3, 2020
@eddyb eddyb deleted the debuginfo-early-create-var branch February 3, 2020 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants