-
Notifications
You must be signed in to change notification settings - Fork 62
feat: memory access adapters, boundary chips, merkle chip E3 #1640
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
feat: memory access adapters, boundary chips, merkle chip E3 #1640
Conversation
…o fix a bunch of bugs
self.control | ||
.execute_instruction(state, &mut self.chip_complex)?; | ||
.execute_instruction(state, &instruction.clone(), &mut self.chip_complex)?; |
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.
how come it's both reference and clone?
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.
It's because
- design-wise I prefer passing a reference
- currently it's hard to do without tricks, because this reference is inside
self.something
, andself.execute_instruction
requires a mutable reference toself
. That is why I did this hack and added a// TODO
above
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 meant does &instruction
not work?
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.
No
Because it is an immutable reference to something inside &self
And execute_instruction(&mut self, ...)
) { | ||
let ptr = pointer / ALIGN; | ||
let meta = unsafe { self.meta.get_unchecked_mut(address_space) }; | ||
meta.set( |
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.
is this faster or should we still do a pre-check and return early when block_size == ALIGN
?
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.
What would we do in this case? We still want to set the meta block. Or do you think that iterating over 1..1
is slower than branch prediction in practice?
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.
oh hm I somehow thought we didn't need to set the meta block in this case, but I guess you need to update the timestamp regardless?
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.
LGTM except comments around (1) avoiding const generic ALIGN
when possible because (2) the boundary memories need to account for per-address space min block size.
Resolves INT-3801.
Vec
how it's done now,get_f
too often (although I don't know how to avoid it normally).VmChipTestBuilder
now has::default_persistent
, so all tests inextensions/rv32im/circuit
pass both with volatile and persistent memory interface.