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

Add Compute_ranges pass #2291

Merged
merged 14 commits into from Mar 18, 2019

Conversation

@mshinwell
Copy link
Contributor

commented Mar 6, 2019

This pull request adds a generic pass for computing (dis)contiguous ranges of Linearize code, delimited by labels, from per-instruction information. There are three applications for the DWARF work:

  1. To coalesce the register availability information from Available_regs into ranges. These are very similar to DWARF location lists and will turn into such.

  2. Like the above, but for variables bound by phantom lets.

  3. To take the annotations on each instruction that indicate what block the instruction lives in and infer ranges. These ranges correspond to DWARF lexical block ranges. (A "block" in our terminology will provide an approximation to source-level scoping, currently deduced from Lambda code, including information about inlined-out frames.)

[*] To be contained within a new version of Debuginfo.t, to be presented in due course.

@lthls This is for you to review as agreed. I haven't put the functor instantiations in this GPR as there are unmet dependencies, but those should not prevent us from merging this piece. Please look at mshinwell/gdb-names-gpr for the instantiations: Available_ranges_vars, Available_ranges_phantom_vars and Lexical_block_ranges.

@lthls

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

I've sent you a pull request (mshinwell#15) containing my review (as CR comments) plus some documentation that I wrote while reviewing. Feel free to rework and reuse the documentation comments if you want to.

I believe the addition to the Identifiable.Tbl interface could actually be removed from this PR, since its only use is in a place where I feel it is not useful.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

Thanks, I shall look at this.

@mshinwell mshinwell force-pushed the mshinwell:compute_ranges branch from 6cd0317 to 309d763 Mar 11, 2019

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

@lthls Please review again, I think I've fixed everything. There is one changeset per CR comment.

@lthls

lthls approved these changes Mar 12, 2019

Copy link
Contributor

left a comment

Thanks for the fixes.
This PR is not useful on its own, but it's harmless and will facilitate other incoming PRs, so I'm in favour of merging it early.

asmcomp/debug/compute_ranges_intf.ml Outdated Show resolved Hide resolved

@mshinwell mshinwell force-pushed the mshinwell:compute_ranges branch from 1f3cf16 to 031b0b7 Mar 12, 2019

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

This is indeed blocking other PRs, so I'd like to merge this right away, once CI passes. Thanks for the review.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

Could someone else please approve merging of this change? It doesn't require any further detailed technical review, I don't think (but @lthls is not formally on the approvers list).

@mshinwell mshinwell force-pushed the mshinwell:compute_ranges branch from 031b0b7 to 4aaa7e0 Mar 15, 2019

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

@gasche This has been rebased, please approve.

@gasche

gasche approved these changes Mar 15, 2019

Copy link
Member

left a comment

Approved on @lthls' behalf -- as I had privately mentioned during the github-is-frozen period.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

I'm going to merge this; it had passed AppVeyor before the rebase, and it looks to me as if there's something broken with AppVeyor at the moment (#8506 failed too, for example.)

Update: actually, no, this does seem to be a genuine failure. I have no idea how Travis passed though.

@mshinwell mshinwell force-pushed the mshinwell:compute_ranges branch from 4aaa7e0 to 95dc984 Mar 15, 2019

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

@gasche (or anyone else) If you see the CI has gone green for this, please merge - thanks.

@mshinwell mshinwell merged commit e3a62ee into ocaml:trunk Mar 18, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.