Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[NLL] convert NLL Region representation into a bit set #45670

Closed
nikomatsakis opened this issue Oct 31, 2017 · 8 comments
Closed

[NLL] convert NLL Region representation into a bit set #45670

nikomatsakis opened this issue Oct 31, 2017 · 8 comments
Labels
E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

The present set of NLL patches uses a terribly inefficient representation for regions consisting of two btrees. I think what we probably want is to use a librustc_data_structures::bitvec::BitMatrix, which is a 2D matrix where each cell can be a boolean. The "rows" here would be the region indices and the "columns" would be the locations + free regions.

As of #45668, the representation of Region is encapsulated within the region_context.rs module, so this change should be fairly easy to make. The basic idea would be that, when we initialize the RegionInferenceContext, we have on hand everything we need to allocate the BitMatrix -- that is, we know how many rows and columns we will need.

So the basic steps would be:

  • Create a map from each free region to its column index
    • For convenience, these may want to be the same as the free region's variable index (which also start from 0).
  • Create a map from each MIR Location to its column index.
    • These would start from the column indices used for free regions.
    • The code to enumerate locations can be found in init_free_regions; it is basically a loop over the basic blocks and then the locations within each basic block.
  • Remove the value field from RegionDefinition
  • Adjust users of the value field like region_contains_point, this should be largely straightforward
  • We'll have to adjust the bitset propagation code -- this could be tricky since it has to copy from one row into another, but I think BitMatrix already supports some APIs for that, we may want to add a few more or just tweak how DFS works
@nikomatsakis nikomatsakis added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-nll labels Oct 31, 2017
@zackmdavis
Copy link
Member

I volunteer.

@nikomatsakis
Copy link
Contributor Author

@zackmdavis great! you may want to start building from #45668, or just wait till it lands

@nikomatsakis
Copy link
Contributor Author

@zackmdavis it landed =) Note however that the new PR #45825 also touches the relevant code, though I don't think it should make much difference to this change.

@nikomatsakis
Copy link
Contributor Author

Note the new policy on landing things for NLL -- basically, use the nll-master branch on my fork for now, and I will take care of the rest.

@nikomatsakis
Copy link
Contributor Author

@zackmdavis have you had a chance to poke at this?

@zackmdavis
Copy link
Member

@nikomatsakis I took a look, but don't have anything to submit yet; I regret the delay. Understood re nll-master integration branch; stand by

@zackmdavis
Copy link
Member

(current WIP is still zackmdavis/rust@d5d4311105; current plan is to address comments Monday; motivated to avert stereotype of flaky, unreliable open-source contributor)

nikomatsakis pushed a commit to zackmdavis/rust that referenced this issue Nov 22, 2017
This should be more efficient than allocating two BTreeSets for every
region variable?—as it is written in rust-lang#45670.
nikomatsakis pushed a commit to nikomatsakis/rust that referenced this issue Nov 22, 2017
This should be more efficient than allocating two BTreeSets for every
region variable?—as it is written in rust-lang#45670.
nikomatsakis pushed a commit to nikomatsakis/rust that referenced this issue Nov 27, 2017
This should be more efficient than allocating two BTreeSets for every
region variable?—as it is written in rust-lang#45670.
@nikomatsakis
Copy link
Contributor Author

This is done.

nikomatsakis pushed a commit to nikomatsakis/rust that referenced this issue Dec 2, 2017
This should be more efficient than allocating two BTreeSets for every
region variable?—as it is written in rust-lang#45670.
nikomatsakis pushed a commit to nikomatsakis/rust that referenced this issue Dec 4, 2017
This should be more efficient than allocating two BTreeSets for every
region variable?—as it is written in rust-lang#45670.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

2 participants