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: correctly transform memory_index mappings for generators. #62072

Merged
merged 1 commit into from
Jun 26, 2019

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jun 23, 2019

Fixes #61793, closes #62011 (previous attempt at fixing #61793).

During #60187, I made the mistake of suggesting that the (re-)computation of memory_index in ty::layout, after generator-specific logic split/recombined fields, be done off of the offsets of those fields (which needed to be computed anyway), as opposed to the memory_index.

memory_index maps each field to its in-memory order index, which ranges over the same 0..n values as the fields themselves, making it a bijective mapping, and more specifically a permutation (indeed, it's the permutation resulting from field reordering optimizations).

Each field has an unique "memory index", meaning a sort based on them, even an unstable one, will not put them in the wrong order. But offsets don't have that property, because of ZSTs (which do not increase the offset), so sorting based on the offset of fields alone can (and did) result in wrong orders.

Instead of going back to sorting based on (slices/subsets of) memory_index, or special-casing ZSTs to make sorting based on offsets produce the right results (presumably), as #62011 does, I opted to drop sorting altogether and focus on O(n) operations involving permutations:

  • a permutation is easily inverted (see the invert_mapping fn)
    • an inverse_memory_index was already employed in other parts of the ty::layout code (that is, a mapping from memory order to field indices)
    • inverting twice produces the original permutation, so you can invert, modify, and invert again, if it's easier to modify the inverse mapping than the direct one
  • you can modify/remove elements in a permutation, as long as the result remains dense (i.e. using every integer in 0..len, without gaps)
    • for splitting a 0..n permutation into disjoint 0..x and x..n ranges, you can pick the elements based on a i < x / i >= x predicate, and for the latter, also subtract x to compact the range to 0..n-x
    • in the general case, for taking an arbitrary subset of the permutation, you need a renumbering from that subset to a dense 0..subset.len() - but notably, this is still O(n)!
  • you can merge permutations, as long as the result remains disjoint (i.e. each element is unique)
    • for concatenating two 0..n and 0..m permutations, you can renumber the elements in the latter to n..n+m
  • some of these operations can be combined, and an inverse mapping (be it a permutation or not) can still be used instead of a forward one by changing the "domain" of the loop performing the operation

I wish I had a nicer / more mathematical description of the recombinations involved, but my focus was to fix the bug (in a way which preserves information more directly than sorting would), so I may have missed potential changes in the surrounding generator layout code, that would make this all more straight-forward.

r? @tmandry

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 23, 2019
@rust-highfive

This comment has been minimized.

@eddyb

This comment has been minimized.

Copy link
Member

@tmandry tmandry left a comment

Choose a reason for hiding this comment

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

r=me after my comment.

Looks good, I knew there was a smarter way of doing it but sorting was so much easier ;)

I think my original plan was to replace sorting of the combined memory indices with a merge sort, which would also be O(n). (Or maybe I hallucinated that after the fact, I can’t remember.) I think both ways would work. Regardless, I’m happy to get this back to linear time.

src/librustc/ty/layout.rs Show resolved Hide resolved
@eddyb
Copy link
Member Author

eddyb commented Jun 25, 2019

@bors r=tmandry

@bors
Copy link
Contributor

bors commented Jun 25, 2019

📌 Commit fad27df has been approved by tmandry

@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 Jun 25, 2019
@bors
Copy link
Contributor

bors commented Jun 26, 2019

⌛ Testing commit fad27df with merge bdd4bda...

bors added a commit that referenced this pull request Jun 26, 2019
rustc: correctly transform memory_index mappings for generators.

Fixes #61793, closes #62011 (previous attempt at fixing #61793).

During #60187, I made the mistake of suggesting that the (re-)computation of `memory_index` in `ty::layout`, after generator-specific logic split/recombined fields, be done off of the `offsets` of those fields (which needed to be computed anyway), as opposed to the `memory_index`.

`memory_index` maps each field to its in-memory order index, which ranges over the same `0..n` values as the fields themselves, making it a bijective mapping, and more specifically a permutation (indeed, it's the permutation resulting from field reordering optimizations).

Each field has an unique "memory index", meaning a sort based on them, even an unstable one, will not put them in the wrong order. But offsets don't have that property, because of ZSTs (which do not increase the offset), so sorting based on the offset of fields alone can (and did) result in wrong orders.

Instead of going back to sorting based on (slices/subsets of) `memory_index`, or special-casing ZSTs to make sorting based on offsets produce the right results (presumably), as #62011 does, I opted to drop sorting altogether and focus on `O(n)` operations involving *permutations*:
* a permutation is easily inverted (see the `invert_mapping` `fn`)
  * an `inverse_memory_index` was already employed in other parts of the `ty::layout` code (that is, a mapping from memory order to field indices)
  * inverting twice produces the original permutation, so you can invert, modify, and invert again, if it's easier to modify the inverse mapping than the direct one
* you can modify/remove elements in a permutation, as long as the result remains dense (i.e. using every integer in `0..len`, without gaps)
  * for splitting a `0..n` permutation into disjoint `0..x` and `x..n` ranges, you can pick the elements based on a `i < x` / `i >= x` predicate, and for the latter, also subtract `x` to compact the range to `0..n-x`
  * in the general case, for taking an arbitrary subset of the permutation, you need a renumbering from that subset to a dense `0..subset.len()` - but notably, this is still `O(n)`!
* you can merge permutations, as long as the result remains disjoint (i.e. each element is unique)
  * for concatenating two `0..n` and `0..m` permutations, you can renumber the elements in the latter to `n..n+m`
* some of these operations can be combined, and an inverse mapping (be it a permutation or not) can still be used instead of a forward one by changing the "domain" of the loop performing the operation

I wish I had a nicer / more mathematical description of the recombinations involved, but my focus was to fix the bug (in a way which preserves information more directly than sorting would), so I may have missed potential changes in the surrounding generator layout code, that would make this all more straight-forward.

r? @tmandry
@bors
Copy link
Contributor

bors commented Jun 26, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: tmandry
Pushing bdd4bda to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 26, 2019
@bors bors merged commit fad27df into rust-lang:master Jun 26, 2019
@eddyb eddyb deleted the generator-memory-index branch June 26, 2019 09:35
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.

Codegen ICE/regression with 2019-06-12 nightly when using async fn<T: Fn()>(&self, T)
5 participants