Skip to content

BitSlice fixes #52335

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

Merged
merged 3 commits into from
Jul 17, 2018
Merged

BitSlice fixes #52335

merged 3 commits into from
Jul 17, 2018

Conversation

nnethercote
Copy link
Contributor

propagate_bits_into_entry_set_for and BitSlice::bitwise are hot for some benchmarks under NLL. I tried and failed to speed them up. (Increasing the size of bit_slice::Word from usize to u128 caused a slowdown, even though decreasing the size of bitvec::Word from u128 to u64 also caused a slowdown. Weird.)

Anyway, along the way I fixed up several problems in and around the BitSlice code.

r? @nikomatsakis

It has a single callsite, and duplicates some code from that callsite.
The code is more concise and clearer this way.
Currently `Word` is `usize`, and there are various places in the code
that assume this.

This patch mostly just changes `usize` occurrences to `Word`. Most of
the changes were found as compile errors when I changed `Word` to a type
other than `usize`, but there was one non-obvious case in
librustc_mir/dataflow/mod.rs that caused bounds check failures before I
fixed it.
In multiple ways:

- Two calls to `bits_to_string()` passed in byte lengths rather than bit
  lengths, which meant only 1/8th of the `BitSlice` was printed.

- `bit_str`'s purpose is entirely mysterious. I removed it and changed
  its callers to print the indices in the obvious way.

- `bits_to_string`'s inner loop was totally wrong, such that it printed
  entirely bogus results.

- `bits_to_string` now also adds a '|' between words, which makes the
  output easier to read, e.g.:
  `[ff-ff-ff-ff-ff-ff-ff-ff|ff-ff-ff-ff-ff-ff-ff-07]`.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 13, 2018
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

seems good :)

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 16, 2018

📌 Commit f2b0b67 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 Jul 16, 2018
@bors
Copy link
Collaborator

bors commented Jul 17, 2018

⌛ Testing commit f2b0b67 with merge 2ddc0cb...

bors added a commit that referenced this pull request Jul 17, 2018
`BitSlice` fixes

`propagate_bits_into_entry_set_for` and `BitSlice::bitwise` are hot for some benchmarks under NLL. I tried and failed to speed them up. (Increasing the size of `bit_slice::Word` from `usize` to `u128` caused a slowdown, even though decreasing the size of `bitvec::Word` from `u128` to `u64` also caused a slowdown. Weird.)

Anyway, along the way I fixed up several problems in and around the `BitSlice` code.

r? @nikomatsakis
@bors
Copy link
Collaborator

bors commented Jul 17, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 2ddc0cb to master...

@bors bors merged commit f2b0b67 into rust-lang:master Jul 17, 2018
@nnethercote nnethercote deleted the BitSlice-fixes branch July 18, 2018 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants