cranelift: Compress vcode range-lists #8506
Conversation
These lists of ranges always cover contiguous ranges of an index space, meaning the start of one range is the same as the end of the previous range, so we can cut storage in half by only storing one endpoint of each range. This in turn means we don't have to keep track of the other endpoint while building these lists, reducing the state we need to keep while building vcode and simplifying the various build steps.
jameysharp
force-pushed
the
compress-range-lists
branch
from
faa7231
to
b528024
Compare
| /// Reverse this list of ranges, so that the first range is at the | ||
| /// last index and the last range is at the first index. | ||
| /// | ||
| /// ```ignore |
There was a problem hiding this comment.
Because doc-tests don't work in non-public types, since they're compiled in a separate crate. I couldn't figure out a better way to document what these methods do than with code, so doc-test style seemed good, but didn't quite work how I wanted. I did build this with the module made public and the doc-tests un-ignored, and verified that they pass.
| /// Get the range at the specified index. | ||
| pub fn get(&self, index: usize) -> Range<usize> { | ||
| let len = self.len(); | ||
| assert!(index < len, "index {index} is too big for length {len}"); | ||
| let index = self.map_index(index); | ||
| self.ranges[index] as usize..self.ranges[index + 1] as usize | ||
| } |
There was a problem hiding this comment.
Can we add some typed indices for Ranges? Or reuse cranelift_entity::Entity?
I've always been slightly nervous about all of our raw indices in the backends.
There was a problem hiding this comment.
I guess we could add some sort of type-system tagging, but what we have now is usize almost everywhere this is used, so it would be noisy.
That said, I had wanted to somehow associate each instance of Ranges with the Vec or Ranges that it indexes. I couldn't figure out a good way to do that, but putting a newtype wrapper around the indexes could work, with some helpers.
I'd prefer to land this without that change, since I believe it's easier to understand and no less safe than the status quo, but I do think this is worth thinking about more.
github-actions
bot
added
cranelift
These lists of ranges always cover contiguous ranges of an index space, meaning the start of one range is the same as the end of the previous range, so we can cut storage in half by only storing one endpoint of each range.
This in turn means we don't have to keep track of the other endpoint while building these lists, reducing the state we need to keep while building vcode and simplifying the various build steps.