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

MachInst backend: pass through SourceLoc information. #1575

Merged
merged 1 commit into from
Apr 24, 2020

Conversation

cfallin
Copy link
Member

@cfallin cfallin commented Apr 22, 2020

This change adds SourceLoc information per instruction in a VCode<Inst>
container, and keeps this information up-to-date across register allocation
and branch reordering. The information is initially collected during
instruction lowering, eventually collected on the MachSection, and finally
provided to the environment that wraps the codegen crate for wasmtime.

This PR is based on top of #1570 and #1571 (part of a series fixing tests).

This PR depends on bytecodealliance/regalloc.rs#50, a change to the register
allocator to provide instruction-granularity info on the rewritten
instruction stream (rather than block-granularity). I have not yet
updated cranelift/codegen/Cargo.toml; we will need to land that PR
and do a release first.

With the prior PRs applied as well, quite a few more unit tests pass;
the exclusion list in #1526 should be updated if this PR lands first.

Addresses part of #1523 (debug info), though debug info consists of
more than just source-location mapping.

@cfallin cfallin added the cranelift:area:aarch64 Issues related to AArch64 backend. label Apr 22, 2020
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:module labels Apr 22, 2020
@github-actions
Copy link

Subscribe to Label Action

cc @bnjbvr

This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:module"

Thus the following users have been cc'd because of the following labels:

  • bnjbvr: cranelift

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Contributor

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

It would be nice if it also said where the prologue ends and where the epilogue starts.

cranelift/codegen/src/machinst/sections.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/vcode.rs Outdated Show resolved Hide resolved
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Looking good overall, I think we may be able to use strategies that use less memory, let me know what you think!

cranelift/codegen/src/machinst/sections.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/sections.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/vcode.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/sections.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/sections.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/vcode.rs Show resolved Hide resolved
cranelift/codegen/src/machinst/sections.rs Outdated Show resolved Hide resolved
@bnjbvr
Copy link
Member

bnjbvr commented Apr 22, 2020

(To be clear, I only commented the third commit which is passing SourceLoc information)

@cfallin
Copy link
Member Author

cfallin commented Apr 23, 2020

Thanks! Updated, PTAL.

@bjorn3:

It would be nice if it also said where the prologue ends and where the epilogue starts.

Sure, that'd be useful metadata as well; but it seems there's no downstream consumer of that at the moment? I suppose it might be better to wait until we have a need for it (or build out more detailed debuginfo infra)...

@bnjbvr bnjbvr self-requested a review April 23, 2020 09:27
@bjorn3
Copy link
Contributor

bjorn3 commented Apr 23, 2020

That would be https://github.com/bjorn3/rustc_codegen_cranelift/issues/937, though in case of the prologue I will likely want to include argument stores generated by cg_clif.

@cfallin
Copy link
Member Author

cfallin commented Apr 23, 2020

@bjorn3 OK, that seems like a reasonable addition. If you don't mind, I think we should postpone that for a followup PR, so that we can land this and get to full test correctness first.

Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

This change adds SourceLoc information per instruction in a `VCode<Inst>`
container, and keeps this information up-to-date across register allocation
and branch reordering. The information is initially collected during
instruction lowering, eventually collected on the MachSection, and finally
provided to the environment that wraps the codegen crate for wasmtime.
@cfallin cfallin merged commit 6cd92f9 into bytecodealliance:master Apr 24, 2020
cfallin added a commit to cfallin/wasmtime that referenced this pull request Apr 29, 2020
Previously, the SourceLoc information transferred in `VCode` only
included PC-spans for non-default SourceLocs. I realized that the
invariant we're supposed to keep here is that every PC is covered; if no
source information, just use `SourceLoc::default()`.

This was spurred by @bjorn3's comment in bytecodealliance#1575 (thanks!).
cfallin added a commit to cfallin/wasmtime that referenced this pull request Apr 30, 2020
Previously, the SourceLoc information transferred in `VCode` only
included PC-spans for non-default SourceLocs. I realized that the
invariant we're supposed to keep here is that every PC is covered; if no
source information, just use `SourceLoc::default()`.

This was spurred by @bjorn3's comment in bytecodealliance#1575 (thanks!).
cfallin added a commit to cfallin/wasmtime that referenced this pull request Apr 30, 2020
Previously, the SourceLoc information transferred in `VCode` only
included PC-spans for non-default SourceLocs. I realized that the
invariant we're supposed to keep here is that every PC is covered; if no
source information, just use `SourceLoc::default()`.

This was spurred by @bjorn3's comment in bytecodealliance#1575 (thanks!).
@cfallin cfallin deleted the test-fixes branch May 6, 2020 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:module cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants