Implement multi-context memory #377
Conversation
bobbinth
force-pushed
the
mem-context
branch
4 times, most recently
from
1fcef7e
to
2617ca7
Compare
bobbinth
force-pushed
the
mem-context
branch
from
55099d5
to
3541262
Compare
bobbinth
force-pushed
the
mem-context
branch
from
3541262
to
dbc4368
Compare
| pub fn from_ints(ctx: u32, addr: Felt, clk: u32, old_word: Word, new_word: Word) -> Self { | ||
| Self { | ||
| ctx: Felt::from(ctx), | ||
| addr, | ||
| clk: Felt::from(clk), | ||
| old_word, | ||
| new_word, | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: It's a bit strange that the type of addr is Felt here when the function name is from_ints (though I understand it, given the source is essentially the io_ops functions). However, it prompted me to look at things a bit more, and the types we use for addr throughout the memory module seem to be kind of inconsistent. For example, in the segment we use u64 for the map and for the get_value methods, but it seems like we always call as_int when passing the input for any calls that aren't coming from tests. In fact, it seems like those get_value methods are exclusively used for tests, with the one exception of get_old_value (this includes methods all the way up the chain, from segment to memory to processor). I don't really have a particular suggestion or change request, but something bothers me a bit here
There was a problem hiding this comment.
The issue here is that when requests come from the stack, address is a Felt - but we can't use Felt's ask keys in the map since they don't implement the required traits. So, we need to convert Felt's to u64. We could change the interface to always take u64 and then the conversion would happen on the stack side - but I'd like to put more thought in whether this is the best option.
I made a couple of minor changes to make the interfaces a bit more consistent - but this does not fix the underlying issue.
There was a problem hiding this comment.
Yep, this makes sense. However, I think the point still stands about the get_value method and all the methods that reference it (except get_old_value). I think we have a bunch of methods that could possibly be compiled for tests only (#[cfg(test)]). There may be a restriction on this due to the integration tests. I'm not sure. But it seems wasteful to be including things if they're only used by tests
bobbinth
force-pushed
the
mem-context
branch
from
dbc4368
to
f6a1c1f
Compare
bobbinth
force-pushed
the
mem-context
branch
from
f6a1c1f
to
31f825e
Compare
This PR adds support for multiple memory contexts. Specifically:
callblock is entered/exited.This PR does not yet include:
CALLoperation is executed.ctx,fmtpand stack depth into block stack table.These will be addressed in future PRs.