-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Tests for cache system #230
Conversation
61570c8
to
4f4a01d
Compare
b3ff7c2
to
3ce5f16
Compare
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.
Overall it looks good!
wasmtime-environ/src/cache.rs
Outdated
} | ||
} | ||
|
||
// cranelift's types (including SecondaryMap) doesn't implement PartialEq |
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 know I originally suggested using this approach, but I actually think we can go back and implement Eq
/PartialEq
for SecondaryMap
, with the approach of trimming the trailing default-value elements. That should hopefully allow the code here to be much simpler.
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.
As we discussed, I'll do it as a follow up refactor later, also implementing the serialization for SecondaryMap
.
wasmtime-environ/src/cache.rs
Outdated
} | ||
} | ||
|
||
// binemit::Reloc doesn't implement PartialEq |
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.
We should make binemit::Reloc
implement PartialEq
and Eq
.
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.
Same thing as with SecondaryMap
- I'll do it later as follow up refactor.
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.
Looks good!
* Fix wasmi no_std support (bytecodealliance#218) Two issues are fixed: 1. num-bigint 0.2 requires std, and is used to perform float to int conversions since bytecodealliance#186. This patch disables this change in no_std environments. This change can be reverted once num-bigint 0.3 is released, which will support no_std. 2. The `f{32,64}::fract` method is currently not implemented by core, only std. This patch uses the no_std supporting implementation from num-trait's FloatCore trait. * Ensure wasmi builds without std in CI The no-std-check cargo subcommand uses a custom sysroot to prevent the crate from using std while checking a local target. This should help ensure that changes which introduce std dependencies are caught at review time.
feat: add ISA specific setting `instrument_inst` for zkASM
Based on #223. Draft to avoid merging before #223.