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

rustc_codegen_ssa: introduce MIR VarDebugInfo, but only for codegen. #65718

Merged
merged 7 commits into from
Nov 1, 2019

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Oct 23, 2019

These are all the codegen changes necessary for #56231.

The refactors were performed locally to codegen, and in several steps, to ease reviewing and avoid introducing changes in behavior (as I'm not sure our debuginfo tests cover enough).

r? @michaelwoerister cc @nagisa @rkruppe @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 23, 2019
src/librustc_codegen_ssa/mir/debuginfo.rs Outdated Show resolved Hide resolved
src/librustc_codegen_ssa/mir/debuginfo.rs Outdated Show resolved Hide resolved
src/librustc_codegen_ssa/mir/debuginfo.rs Outdated Show resolved Hide resolved
@bors

This comment has been minimized.

@eddyb
Copy link
Member Author

eddyb commented Oct 24, 2019

As per #56231 (comment):

@bjorn3 Please leave comments on the commit in the PR's commit list, not on the commit itself, because I can't reply to the latter from the PR view, and they might even disappear.

@bors

This comment has been minimized.

@eddyb
Copy link
Member Author

eddyb commented Oct 29, 2019

r? @nikomatsakis

src/librustc_codegen_ssa/mir/debuginfo.rs Outdated Show resolved Hide resolved
@@ -149,54 +174,107 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
}
}

pub fn debug_declare_locals(&self, bx: &mut Bx) {
let tcx = self.cx.tcx();
/// Apply debuginfo and/or name, after creating the `alloca` for a local,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 for doc-comments

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 29, 2019

📌 Commit 2994186 has been approved by nikomatsakis

@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 Oct 29, 2019
@nikomatsakis
Copy link
Contributor

@bors r-

Until @eddyb addresses comments by @bjorn3

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

eddyb commented Oct 30, 2019

Oops, I forgot that @bjorn3's comments were all about pre-existing problems, in the code I moved from mod.rs to debuginfo.rs (in src/librustc_codegen_ssa/mir).

@bors r=nikomatsakis rollup=never

@bors
Copy link
Contributor

bors commented Oct 30, 2019

📌 Commit 2994186 has been approved by nikomatsakis

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 30, 2019
@bors
Copy link
Contributor

bors commented Oct 31, 2019

⌛ Testing commit 2994186 with merge 404d42f...

bors added a commit that referenced this pull request Oct 31, 2019
rustc_codegen_ssa: introduce MIR VarDebugInfo, but only for codegen.

These are all the codegen changes necessary for #56231.

The refactors were performed locally to codegen, and in several steps, to ease reviewing and avoid introducing changes in behavior (as I'm not sure our debuginfo tests cover enough).

r? @michaelwoerister cc @nagisa @rkruppe @oli-obk
@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 31, 2019
@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 31, 2019
@eddyb
Copy link
Member Author

eddyb commented Oct 31, 2019

Weird, why does wasm get no temporaries here? Are we more aggressive somehow?
Do we not put vectors behind pointers in wasm, and do on other targets, by any chance?

EDIT: more exactly, what's happening here is that on all targets %x is an LLVM parameter of the function, but then when the intrinsic is called:

  • most targets use %_2, which was created from %x (presumably via load?)
  • wasm uses %x directly (so the function signature surely takes it by value)

I can trivially adjust the test so both forms are allowed, but I'm suspicious of the behavior.

cc @alexcrichton @tlively

@alexcrichton
Copy link
Member

@eddyb this looks simd related and wasm-specific so it's likely related to this configuration option where simd values are passed by-value in wasm and not by-reference, because wasm has entirely different validation/register rules than traditional elf executables.

Perhaps it's safe to just ignore the test on wasm?

@tlively
Copy link
Contributor

tlively commented Oct 31, 2019

In one of my recent PRs I generalized one of the SIMD tests by replacing the [0-9]+ with [0-9a-z]+ so it would match x as well. Perhaps it would make sense to do the same here?

@eddyb
Copy link
Member Author

eddyb commented Oct 31, 2019

@alexcrichton Thanks, that definitely explains it!

@tlively I'll use %{{x|_2}} (and similar), I think, and leave a comment explaining why that is, to avoid any surprises in the future.

@eddyb
Copy link
Member Author

eddyb commented Oct 31, 2019

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Oct 31, 2019

📌 Commit 60a2266 has been approved by nikomatsakis

@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 Oct 31, 2019
@bors
Copy link
Contributor

bors commented Nov 1, 2019

⌛ Testing commit 60a2266 with merge 01e5d91...

bors added a commit that referenced this pull request Nov 1, 2019
rustc_codegen_ssa: introduce MIR VarDebugInfo, but only for codegen.

These are all the codegen changes necessary for #56231.

The refactors were performed locally to codegen, and in several steps, to ease reviewing and avoid introducing changes in behavior (as I'm not sure our debuginfo tests cover enough).

r? @michaelwoerister cc @nagisa @rkruppe @oli-obk
@bors
Copy link
Contributor

bors commented Nov 1, 2019

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing 01e5d91 to master...

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.

8 participants