-
Notifications
You must be signed in to change notification settings - Fork 1k
Move SAR, SHR and SHL to UInt256 #10216
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
base: main
Are you sure you want to change the base?
Changes from all commits
65b3c23
5a8a8f5
833a2e7
2dfdae0
5187470
f4ed77d
3fe50fa
065670f
68dcd2b
da3fa5b
23bfc88
809c201
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -625,6 +625,186 @@ public static byte[] sub(final byte[] x, final byte[] y) { | |||||
| return sbb(x, y); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Performs EVM SAR (arithmetic shift right) on the two top stack items. | ||||||
| * | ||||||
| * <p>Reads the shift amount (unsigned) and the value (signed) from the top two stack slots, | ||||||
| * writes {@code value >> shift} back into the value slot and decrements the top. Shifts >= 256 | ||||||
| * produce 0 for positive values and -1 for negative values. | ||||||
| * | ||||||
| * @param shift 256 bit value storing the amount of bits to shift | ||||||
| * @return the result | ||||||
| */ | ||||||
| public UInt256 sar(final UInt256 shift) { | ||||||
| int bitShift; | ||||||
| if (shift.u3() != 0 | ||||||
| || shift.u2() != 0 | ||||||
| || shift.u1() != 0 | ||||||
| || Long.compareUnsigned(shift.u0(), 256) >= 0) { | ||||||
| bitShift = 256; | ||||||
| } else { | ||||||
| bitShift = (int) shift.u0(); | ||||||
| } | ||||||
| long fill = (u3 < 0 ? -1L : 0); | ||||||
| return sar0(bitShift, fill); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Performs EVM SHR (logical shift right) on the two top stack items. | ||||||
| * | ||||||
| * <p>Reads the shift amount (unsigned) and the value from the top two stack slots, writes {@code | ||||||
| * value >>> shift} back into the value slot and decrements the top. Shifts >= 256 or a zero value | ||||||
| * produce 0. | ||||||
| * | ||||||
| * @param shift 256 bit value storing the amount of bits to shift | ||||||
| * @return the result | ||||||
| */ | ||||||
| public UInt256 shr(final UInt256 shift) { | ||||||
| int bitShift; | ||||||
| if (shift.u3() != 0 | ||||||
| || shift.u2() != 0 | ||||||
| || shift.u1() != 0 | ||||||
| || Long.compareUnsigned(shift.u0(), 256) >= 0) { | ||||||
| bitShift = 256; | ||||||
| } else { | ||||||
| bitShift = (int) shift.u0(); | ||||||
| } | ||||||
| return sar0(bitShift, 0); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems odd for shr to call a sar method
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why? I think this is elegant. SAR is pretty much a SHR with a custom fill.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So couldn't you equally call it shr0 ? sar and shr imply specific things, their shared code is not sar.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to be clear, I like the structure just not the name
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I disagree there, because a shift with a custom fill is an arithmetic shift so it makes sense for SHR to call an internal SAR with a static fill of 0. |
||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Arithmetic right-shifts a 256-bit value in place by 0..255 bits, sign-extending with {@code | ||||||
| * fill}. | ||||||
| * | ||||||
| * @param shift number of bits to shift | ||||||
| * @param fill value to prepend while shifting | ||||||
| * @return the result | ||||||
| */ | ||||||
| // TODO: check perf - wiring shiftRight callers with this one | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since PR is approved, just pointing out there's still a TODO
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't want to clutter this PR with that, but also want to track this for changing in a follow up. |
||||||
| private UInt256 sar0(final int shift, final long fill) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be renamed to something more descriptive?
|
||||||
| long w3 = u3, w2 = u2, w1 = u1, w0 = u0; | ||||||
| if (shift == 256) { | ||||||
| w3 = fill; | ||||||
| w2 = fill; | ||||||
| w1 = fill; | ||||||
| w0 = fill; | ||||||
| } else if (shift != 0) { | ||||||
| // Number of whole 64-bit words to shift (shift / 64) | ||||||
| final int wordShift = shift >>> 6; | ||||||
| // Remaining intra-word bit shift (shift % 64) | ||||||
| final int bitShift = shift & 63; | ||||||
| switch (wordShift) { | ||||||
| case 0: | ||||||
| w0 = shiftRightWord(w0, w1, bitShift); | ||||||
| w1 = shiftRightWord(w1, w2, bitShift); | ||||||
| w2 = shiftRightWord(w2, w3, bitShift); | ||||||
| w3 = shiftRightWord(w3, fill, bitShift); | ||||||
| break; | ||||||
| case 1: | ||||||
| w0 = shiftRightWord(w1, w2, bitShift); | ||||||
| w1 = shiftRightWord(w2, w3, bitShift); | ||||||
| w2 = shiftRightWord(w3, fill, bitShift); | ||||||
| w3 = fill; | ||||||
| break; | ||||||
| case 2: | ||||||
| w0 = shiftRightWord(w2, w3, bitShift); | ||||||
| w1 = shiftRightWord(w3, fill, bitShift); | ||||||
| w2 = fill; | ||||||
| w3 = fill; | ||||||
| break; | ||||||
| case 3: | ||||||
| w0 = shiftRightWord(w3, fill, bitShift); | ||||||
| w1 = fill; | ||||||
| w2 = fill; | ||||||
| w3 = fill; | ||||||
| break; | ||||||
| } | ||||||
| } | ||||||
| return new UInt256(w3, w2, w1, w0); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Performs EVM SHL (shift left) on the two top stack items. | ||||||
| * | ||||||
| * <p>Reads the shift amount (unsigned) and the value from the top two stack slots, writes {@code | ||||||
| * value << shift} back into the value slot and decrements the top. Shifts >= 256 or a zero value | ||||||
| * produce 0. | ||||||
| * | ||||||
| * @param shift 256 bit value storing the amount of bits to shift | ||||||
| * @return the result | ||||||
| */ | ||||||
| public UInt256 shl(final UInt256 shift) { | ||||||
| int bitShift; | ||||||
| if (shift.u3() != 0 | ||||||
| || shift.u2() != 0 | ||||||
| || shift.u1() != 0 | ||||||
| || Long.compareUnsigned(shift.u0(), 256) >= 0) { | ||||||
| bitShift = 256; | ||||||
| } else { | ||||||
| bitShift = (int) shift.u0(); | ||||||
| } | ||||||
| return shl0(bitShift); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Left-shifts a 256-bit value in place by 1..255 bits, zero-filling from the right. | ||||||
| * | ||||||
| * @param shift number of bits to shift | ||||||
| * @return the result | ||||||
| */ | ||||||
| // TODO: check perf - wiring shiftLeft callers with this one | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||||||
| private UInt256 shl0(final int shift) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this clashes with the current |
||||||
| long w3 = u3, w2 = u2, w1 = u1, w0 = u0; | ||||||
| if (shift == 256) { | ||||||
| w3 = 0; | ||||||
| w2 = 0; | ||||||
| w1 = 0; | ||||||
| w0 = 0; | ||||||
| } else if (shift != 0) { | ||||||
| // Number of whole 64-bit words to shift (shift / 64) | ||||||
| final int wordShift = shift >>> 6; | ||||||
| // Remaining intra-word bit shift (shift % 64) | ||||||
| final int bitShift = shift & 63; | ||||||
| switch (wordShift) { | ||||||
| case 0: | ||||||
| w3 = shiftLeftWord(w3, w2, bitShift); | ||||||
| w2 = shiftLeftWord(w2, w1, bitShift); | ||||||
| w1 = shiftLeftWord(w1, w0, bitShift); | ||||||
| w0 = shiftLeftWord(w0, 0, bitShift); | ||||||
| break; | ||||||
| case 1: | ||||||
| w3 = shiftLeftWord(w2, w1, bitShift); | ||||||
| w2 = shiftLeftWord(w1, w0, bitShift); | ||||||
| w1 = shiftLeftWord(w0, 0, bitShift); | ||||||
| w0 = 0; | ||||||
| break; | ||||||
| case 2: | ||||||
| w3 = shiftLeftWord(w1, w0, bitShift); | ||||||
| w2 = shiftLeftWord(w0, 0, bitShift); | ||||||
| w1 = 0; | ||||||
| w0 = 0; | ||||||
| break; | ||||||
| case 3: | ||||||
| w3 = shiftLeftWord(w0, 0, bitShift); | ||||||
| w2 = 0; | ||||||
| w1 = 0; | ||||||
| w0 = 0; | ||||||
| break; | ||||||
| } | ||||||
| } | ||||||
| return new UInt256(w3, w2, w1, w0); | ||||||
| } | ||||||
|
|
||||||
| private static long shiftLeftWord(final long value, final long nextValue, final int bitShift) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add javadoc. |
||||||
| if (bitShift == 0) return value; | ||||||
| return (value << bitShift) | (nextValue >>> (64 - bitShift)); | ||||||
| } | ||||||
|
|
||||||
| private static long shiftRightWord(final long value, final long prevValue, final int bitShift) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add javadoc. |
||||||
| if (bitShift == 0) return value; | ||||||
| return (value >>> bitShift) | (prevValue << (64 - bitShift)); | ||||||
| } | ||||||
|
|
||||||
| private static boolean isZero(final byte[] arr) { | ||||||
| int index = Arrays.mismatch(arr, ZERO_BYTES); | ||||||
| return (index == -1 || index >= arr.length); | ||||||
|
|
@@ -1915,4 +2095,5 @@ record UInt576(long u8, long u7, long u6, long u5, long u4, long u3, long u2, lo | |||||
|
|
||||||
| // -------------------------------------------------------------------------- | ||||||
| // endregion | ||||||
|
|
||||||
| } | ||||||
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.
Did you consider
?
Uh oh!
There was an error while loading. Please reload this page.
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 believe so - I tried multiple things. These methods are heavily optimized, I would recommend you try and see the results - sometimes it may seem like a fast path but it changes how JIT sees the method.