Add div and mod to u256 with their dependencies #86
Conversation
assembly/integer/u256.ts
Outdated
|
|
||
| const shiftAmount: i32 = i32(u128.clz(divisor)); | ||
|
|
||
| const normalizedDivisor: u128 = safeShl(divisor, shiftAmount); |
There was a problem hiding this comment.
Using safe shifts is redundant due to clz result will never exceed more than 127 for divisor. 128 also cover by earlier divisor == u128.Zero check.
Basically all safeShl and safeShl doesn't make sense here and just add extra perf and size overhead.
There was a problem hiding this comment.
You're quiet right, except for here:
const normalizedDividendHigh: u128 = safeShl(dividendHigh, shiftAmount) | safeShr(dividendLow, 128 - shiftAmount);When shiftAmount = 0, the code right shits with a value of 128 which should be 0, but as the implementation silently overflow, the returned result is wrong. Instead of just handling specifically this case here, I choose a safe implementation of right and left shift.
Do you prefer to remove everything else and handle this specific case by hand ?
There was a problem hiding this comment.
shiftAmount == 0 could be handled separately. shiftAmount == 0 means divisor == u256.Max which is special case
There was a problem hiding this comment.
Ok I will change the logic then.
As a side note shiftAmount == 0 <=> divisor > 2^128, not necessarily divisor == u256.Max
There was a problem hiding this comment.
As a side note shiftAmount == 0 <=> divisor > 2^128, not necessarily divisor == u256.Max
Yes, but in helper routines which uses u256 / u128 like (div128 and longDivision256by128) divisor has u128 type
assembly/integer/u128.ts
Outdated
| * The function uses the long division principle to divide a 128-bit dividend by a 64-bit divisor. | ||
| * The high and low 64-bit parts of the 128-bit dividend are divided separately by the 64-bit divisor. | ||
| */ | ||
| quoRem(divisor: u64): u128[] { |
There was a problem hiding this comment.
Public quoRem is not necessary. See how all this done for u128.div and u128.rem. I use a common private routine for all of this here: https://github.com/MaxGraey/as-bignum/blob/master/assembly/globals.ts#L248
which later reuse in u128 and i128 here: https://github.com/MaxGraey/as-bignum/blob/master/assembly/integer/u128.ts#L411 and https://github.com/MaxGraey/as-bignum/blob/master/assembly/integer/u128.ts#L418
Also I avoid return tuple and use globals like __divmod_quot_hi, __divmod_rem_lo, __divmod_rem_hi. This avoids GC allocs and improve performance
There was a problem hiding this comment.
Is your comment also concerning div64 or only quoRem ?
There was a problem hiding this comment.
Yes, I think quoRem, div64 and div128 are not necessary. Will be great to have only div(u256, u256) and rem(u256, u256) which also have overloaded operators.
There was a problem hiding this comment.
Ok, but using __udivmod128 instead of div64, modified to match your concerns of using the stack and only exposing div and rem, will certainly be less performant.
There was a problem hiding this comment.
It will be great to follow the same approach as in __udivmod128 and just create __udivmod256 and move all logic from div128 / u256.quoRem / longDivision256by128 into globals.ts
There was a problem hiding this comment.
Ok, but using __udivmod128 instead of div64, modified to match your concerns of using the stack and only exposing div and rem, will certainly be less performant.
Not sure what you mean. AssemblyScript and Wasm doesn't have shadow stack. That's why using temporary mutable globals for returning multi-value result from function is the most performant way
assembly/integer/u256.ts
Outdated
| static shr(value: u256, shift: i32): u256 { | ||
| shift &= 255; | ||
| var off = shift as u64; | ||
| if (shift <= 64) { | ||
| if (shift == 0) return value; | ||
| let hi2 = value.hi2 >> off; | ||
| let hi1 = (value.hi1 >> off) | (value.hi2 << 64 - off); | ||
| let lo2 = (value.lo2 >> off) | (value.hi1 << 64 - off); | ||
| let lo1 = (value.lo1 >> off) | (value.lo2 << 64 - off); | ||
| return new u256(lo1, lo2, hi1, hi2); | ||
| } else if (shift > 64 && shift <= 128) { | ||
| let hi1 = value.hi2 >> 128 - off; | ||
| return new u256(value.lo2, value.hi1, hi1); | ||
| } else if (shift > 128 && shift <= 192) { | ||
| let lo2 = value.hi2 >> 192 - off; | ||
| return new u256(value.hi1, lo2); | ||
| } else { | ||
| return new u256(value.hi2 >> 256 - off); | ||
| if (shift >= 256) return u256.Zero; | ||
| if (shift <= 0) return value; | ||
|
|
||
| if(shift > 128) { | ||
| let low = new u128(value.hi1, value.hi2) >> (shift - 128); | ||
| return new u256(low.lo, low.hi, 0, 0); |
There was a problem hiding this comment.
This is mismatch behavior of u128.shr. We don't use such safe checks for usual u128 / u256. And use wrapping behavior which is expected for all basic shifts including (u32, i32, u64, i64). Safe shifting ops uses only in safe/u128, safe/u256 and etc in this namespace: https://github.com/MaxGraey/as-bignum/tree/master/assembly/integer/safe
So I propose delete this chages. Also doesn't relate to div / rem
There was a problem hiding this comment.
OK, it's consistent with web assembly behaviors.
This subject is linked to the div / rem subject as expalined here.
Depending on your answer there, I will make the proper modification.
|
Hello |
Inconsistency and breaking changes for some existing code |
|
A more closely aligned implementation with the existing code has been proposed in #88. |
Description
This pull request aims to introduce
divandmodoperations to theu256data type as part of Massa's ongoing smart contract development.These functionalities have been specifically requested by one of our partners and are expected to enhance the arithmetic capabilities of our smart contracts.
Changes
u64andu128to prevent overflow issues.u128:div64quoRem(returns quotient and remainder) for au64divisor.u256:div128quoRem, Division, and Modulus functions.Challenges and Solutions
The primary challenge was the sheer number of changes required, along with the potential for overflow in bitwise shift operations. To address this, safe implementations were created for both
u64andu128.Additional Considerations
The algorithms should be performant, as they were inspired by existing Golang implementations. For reference, see here.
However, there may be room for further code factorization.
Additionally, some functions may need to be relocated.