Merge Cairo Pie extra segments into one segment #1960
+258
−17
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
Benchmark Results for unmodified programs 🚀
|
FrancoGiachetta
requested review from
igaray,
Oppen,
fmoletta,
juanbono,
pefontana,
gabrielbosio and
yuvalsw
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1960 +/- ##
==========================================
+ Coverage 96.39% 96.40% +0.01%
==========================================
Files 102 102
Lines 41566 41776 +210
==========================================
+ Hits 40068 40276 +208
- Misses 1498 1500 +2 ☔ View full report in Codecov by Sentry. |
JulianGCalderon
requested changes
Feb 18, 2025
Comment on lines
+658
to
+676
| /// Relocates a `Relocatable` value, which represented by its | ||
| /// index and offset, according to a given segment offsets | ||
| fn relocate_value( | ||
| index: usize, | ||
| offset: usize, | ||
| segment_offsets: &Option<HashMap<usize, Relocatable>>, | ||
| ) -> (usize, usize) { | ||
| if let Some(offsets) = segment_offsets { | ||
| return match offsets.get(&index) { | ||
| Some(relocatable) => ( | ||
| relocatable.segment_index as usize, | ||
| relocatable.offset + offset, | ||
| ), | ||
| None => (index, offset), | ||
| }; | ||
| } | ||
|
|
||
| (index, offset) | ||
| } |
There was a problem hiding this comment.
The function only relocates the value if segment_offsets is not None, ignoring it otherwise. I think its better if we remove the Option and let the caller handle the default case (when there is no segment_offsets)
There was a problem hiding this comment.
That would make the method inconsistent with the python implementation. See:
There was a problem hiding this comment.
That is true, but while we aim to be compatible with the python implementation, I don't think the internal implementation has to be exactly the same. We may change the implementation to better suit the language features, although I'm not 100% sure about it. What do you think @gabrielbosio?
There was a problem hiding this comment.
Ah I see your point. I wasn't talking about "inconsistent" in terms of implementation but rather in terms of the results. This function seems to have only one behavior. So if the default case isn't gonna change, isn't it better to let the function handle it all?
There was a problem hiding this comment.
If you want to use the language features you can do something like this:
fn relocate_value(
index: usize,
offset: usize,
segment_offsets: Option<&HashMap<usize, Relocatable>>,
) -> (usize, usize) {
segment_offsets
.and_then(|offsets| offsets.get(&index))
.and_then(|relocatable| {
Some((
relocatable.segment_index as usize,
relocatable.offset + offset,
))
})
.unwrap_or((index, offset))
}and call to_bytes this way:
self.memory.to_bytes(segment_offsets.as_ref())You can remove the Option in the relocate_value argument but you would need to deal with it outside:
let (segment, offset) = if let Some(offsets) = seg_offsets {
Self::relocate_value(*segment, *offset, offsets)
} else {
(*segment, *offset)
};I'm not sure if there's a cleaner way to deal with the Option value here. I would choose to deal with it inside the function for now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allow merging extra_segments with Cairo Pie to increase the segments number limit
Description
The PR adds the method
merge_extra_segmentsto mergeextra_segmentsinto one segment. This solves the issue of cairo pie limiting the number of segments to2^16. This method returns a tuple with the merged segment and aHasMapwith the new offsets (made out of the old segment indices mapped to their new offset in the new segment). This new offsets are then used during the memory serialization.A new flag
merge_extra_segmentswas added to the cli, which merges the extra segments if there were any. Memory serialization is then done taking into account this new segment. If there were no extra segments, it defaults to normal behavior.Closes #1947
Checklist