compiler-rt: alu: add shl (shift left) and lshr (logical shift right) polyfills
#93
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
iamrecursion
requested changes
Nov 20, 2024
compiler-rt/src/alu/lshr.cairo
Outdated
| assert_fits_in_type::<T>(shift); | ||
|
|
||
| let mut result = n; | ||
| #[cairofmt::skip] |
There was a problem hiding this comment.
Why are we skipping this?
There was a problem hiding this comment.
Because it breaks the loop in an ugly way:
for _ in 0
..shift {
let mut new_result = 0;
let mut mask = Bounded::<T>::MAX.into() / 2 + 1;
for _ in 0
..BitSize::<
T
>::bits() {
if result & mask != 0 {
new_result = new_result | mask / 2; // Shift the mask right
}
mask = mask / 2;
};
result = new_result;
};vs
#[cairofmt::skip]
for _ in 0..shift {
let mut new_result = 0;
let mut mask = Bounded::<T>::MAX.into() / 2 + 1;
for _ in 0..BitSize::<T>::bits() {
if result & mask != 0 {
new_result = new_result | mask / 2; // Shift the mask right
}
mask = mask / 2;
};
result = new_result;
};
wzmuda
force-pushed
the
wz/alu-bit-ops-generics
branch
from
bb80742 to
ad2ac36
Compare
wzmuda
force-pushed
the
wz/alu-bit-ops-shl
branch
2 times, most recently
from
652458b to
654b792
Compare
Both documented and right shift heavily simplified. |
iamrecursion
requested changes
Nov 20, 2024
compiler-rt/src/alu/shl.cairo
Outdated
Comment on lines
39
to
49
| for _ in 0..shift { | ||
| // Initialize new_result to 0 for the current shift. | ||
| let mut new_result = 0; | ||
| // Initialize mask to 0b0000..1 (it will move to the left so we can check each bit). | ||
| let mut mask = 1; | ||
|
|
||
| // Iterate through each bit position of the integer. | ||
| for _ in 0..BitSize::<T>::bits() { | ||
| if result & mask != 0 { | ||
| // If the current bit is set, set the corresponding bit in new_result, | ||
| // but shifted one position to the left. | ||
| // | ||
| // mask.wrapping_add(mask) is essentially mask * 2 or mask << 1 | ||
| // with the benefit of wrapping back at 0 when we reach the MSB. | ||
| new_result = new_result | mask.wrapping_add(mask); | ||
| } | ||
| mask = mask.wrapping_add(mask); | ||
| }; | ||
| result = new_result; | ||
| }; |
There was a problem hiding this comment.
This is a really convoluted way to do it. Can we not just do multiplications by 2?
There was a problem hiding this comment.
If I go with this:
for _ in 0..shift {
result = result *2;
result = result & Bounded::<T>::MAX.into();
};It almost works but fails for 128 bit values because of overflow. Shorter values get away with it because the calculations are done on u128 so there's enough room for overflows but in case of u128 there's no extra bits:
running 10 tests
test compiler_rt::alu::shl::shl_i128::tests::test_i128_shifts_zeros_2 ... ok (gas usage est.: 36136631)
test compiler_rt::alu::shl::shl_i128::tests::test_i128_shifts_ones_1 ... fail (gas usage est.: 362082)
test compiler_rt::alu::shl::shl_i128::tests::test_i128_shifts_zeros_1 ... ok (gas usage est.: 49146900)
test compiler_rt::alu::shl::shl_i128::tests::test_i128_shift_mixed ... fail (gas usage est.: 183026)
test compiler_rt::alu::shl::shl_i32::tests::test_i32 ... ok (gas usage est.: 28904173)
test compiler_rt::alu::shl::shl_i128::tests::test_i128_shifts_ones_2 ... fail (gas usage est.: 183026)
test compiler_rt::alu::shl::shl_i8::tests::test_i8 ... ok (gas usage est.: 9936313)
test compiler_rt::alu::shl::shl_i16::tests::test_i16 ... ok (gas usage est.: 15317749)
test compiler_rt::alu::shl::shl_i1::tests::test_i1 ... ok (gas usage est.: 1204618)
test compiler_rt::alu::shl::shl_i64::tests::test_i64 ... ok (gas usage est.: 67371229)
failures:
compiler_rt::alu::shl::shl_i128::tests::test_i128_shifts_ones_1 - Panicked with 0x753132385f6d756c204f766572666c6f77 ('u128_mul Overflow').
compiler_rt::alu::shl::shl_i128::tests::test_i128_shift_mixed - Panicked with 0x753132385f6d756c204f766572666c6f77 ('u128_mul Overflow').
compiler_rt::alu::shl::shl_i128::tests::test_i128_shifts_ones_2 - Panicked with 0x753132385f6d756c204f766572666c6f77 ('u128_mul Overflow').There was a problem hiding this comment.
It might be worth looking at separate impls then, because the mul approach is way more efficient for the cases where it works.
There was a problem hiding this comment.
Ok, so I'll rework it so the generic function does multiplication and the i128 does the complex stuff
wzmuda
force-pushed
the
wz/alu-bit-ops-generics
branch
from
2d4660a to
fa1b0bb
Compare
wzmuda
force-pushed
the
wz/alu-bit-ops-shl
branch
from
28aad98 to
19e8aaf
Compare
iamrecursion
approved these changes
Nov 20, 2024
wzmuda
force-pushed
the
wz/alu-bit-ops-generics
branch
from
fa1b0bb to
d5f9ec2
Compare
wzmuda
force-pushed
the
wz/alu-bit-ops-shl
branch
from
19e8aaf to
dfc2ab3
Compare
Signed-off-by: Wojciech Zmuda <[email protected]>
Signed-off-by: Wojciech Zmuda <[email protected]>
wzmuda
force-pushed
the
wz/alu-bit-ops-shl
branch
from
dfc2ab3 to
c15a151
Compare
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.
Summary
Both polyfills have very similar logic so I push them in a single PR. They already follow the pattern I introduce in #92 - there's one generic implementation and the concrete implementations are just aliases existing to comply with our design.
This PR is based on #92 to make the diff show only the 2 commits that this PR actually adds. It's important to change the base back to
mainbefore merge and merge this PR after #92.Details
Please check the logic of
lshandlshrfunctions inlsh.cairoandlshr.cairofiles. Test cases are automatically generated (just like I did withand/or/xor) so they're most likely correctChecklist
scarb fmt.[ ] Documentation has been updated if necessary.