-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Introduce the cranelift-bitset
crate; use it for stack maps in both Cranelift and Wasmtime
#8826
Conversation
The eventual goal is to deduplicate bitset types between Cranelift and Wasmtime, especially their use in stack maps.
Mostly for stack maps, also for a variety of other random things where `cranelift_codegen::bitset::BitSet` was previously used.
/// ``` | ||
#[inline] | ||
pub fn contains(&self, i: u8) -> bool { | ||
assert!(i < Self::capacity()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the perf-sensitive nature of bitsets should this be debug_assert!
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that originally, but figured we might as well do the more-correct thing by default and add unchecked
versions later if profiling shows that these asserts actually matter.
I kind of expect LLVM to clean them all up, at least when they are called via CompoundBitSet
methods, since it should be able to determine the ranges of values flowing into these methods based on the modulo operation in CompoundBitSet::word_and_bit
.
Makes a bunch of other stuff easier, and will be needed for replacing `cranelift_entity::EntitySet`'s bitset with this thing anyways.
@alexcrichton this should be ready for a second round of review now. |
No description provided.