-
Notifications
You must be signed in to change notification settings - Fork 187
Added hints felt unpacking for blake #2032
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
Conversation
bd562f4
to
327c665
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.
Reviewable status: 0 of 4 files reviewed, all discussions resolved
@@ -947,13 +958,13 @@ ids.res.d2 = res_split[2]"#}), | |||
a = [] | |||
for _ in range(length): | |||
a.append( num & ((1 << num_bits_shift) - 1) ) | |||
num = num >> num_bits_shift |
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.
revert
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.
Done.
return tuple(a) | ||
|
||
def pack(z, num_bits_shift: int) -> int: | ||
limbs = (z.d0, z.d1, z.d2) | ||
return sum(limb << (num_bits_shift * i) for i, limb in enumerate(limbs)) | ||
|
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.
revert
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.
Done.
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.
Hi @yuvalsw! I left you a minor comment.
Also, some CI jobs seem to be failing.
@@ -426,6 +426,17 @@ segments.write_arg(ids.data + 4, [(ids.high >> (B * i)) & MASK for i in range(4) | |||
MASK = 2 ** 32 - 1 | |||
segments.write_arg(ids.data, [(ids.high >> (B * (3 - i))) & MASK for i in range(4)]) | |||
segments.write_arg(ids.data + 4, [(ids.low >> (B * (3 - i))) & MASK for i in range(4)])"#}), | |||
(IS_MORE_THAN_63_BITS_AND_NOT_END, indoc! {r#"memory[ap] = to_felt_or_relocatable((ids.end != ids.packed_values) and (memory[ids.packed_values] < 2**63))"#}), |
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 python code:
memory[ap] = to_felt_or_relocatable(
(ids.end != ids.packed_values) and
(memory[ids.packed_values] < 2**63)
)
Shouldn't the hint be called IS_LESS_THAN_63_BITS_AND_NOT_END
?
If so, could you update other mentions to this hint?
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.
Done.
327c665
to
71770c9
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.
Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @FrancoGiachetta, @JulianGCalderon, and @YairVaknin-starkware)
@@ -947,13 +958,13 @@ ids.res.d2 = res_split[2]"#}), | |||
a = [] | |||
for _ in range(length): | |||
a.append( num & ((1 << num_bits_shift) - 1) ) | |||
num = num >> num_bits_shift |
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.
Done.
return tuple(a) | ||
|
||
def pack(z, num_bits_shift: int) -> int: | ||
limbs = (z.d0, z.d1, z.d2) | ||
return sum(limb << (num_bits_shift * i) for i, limb in enumerate(limbs)) | ||
|
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.
Done.
Benchmark Results for unmodified programs 🚀
|
71770c9
to
33d88a3
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.
Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @FrancoGiachetta, @JulianGCalderon, and @YairVaknin-starkware)
@@ -426,6 +426,17 @@ segments.write_arg(ids.data + 4, [(ids.high >> (B * i)) & MASK for i in range(4) | |||
MASK = 2 ** 32 - 1 | |||
segments.write_arg(ids.data, [(ids.high >> (B * (3 - i))) & MASK for i in range(4)]) | |||
segments.write_arg(ids.data + 4, [(ids.low >> (B * (3 - i))) & MASK for i in range(4)])"#}), | |||
(IS_MORE_THAN_63_BITS_AND_NOT_END, indoc! {r#"memory[ap] = to_felt_or_relocatable((ids.end != ids.packed_values) and (memory[ids.packed_values] < 2**63))"#}), |
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.
Done.
This PR is an as-is copy of AlonT's PR pushed to a fork repo.
33d88a3
to
a402928
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## starkware-development #2032 +/- ##
=========================================================
+ Coverage 96.52% 96.53% +0.01%
=========================================================
Files 102 102
Lines 42951 43082 +131
=========================================================
+ Hits 41458 41589 +131
Misses 1493 1493 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @FrancoGiachetta and @JulianGCalderon)
This PR is an as-is copy of AlonT's PR pushed to a fork repo.
This PR is an as-is copy of AlonT's PR pushed to a fork repo.
This PR is an as-is copy of AlonT's PR pushed to a fork repo.
This change is