ethnum-rs icon indicating copy to clipboard operation
ethnum-rs copied to clipboard

Div by zero

Open NCGThompson opened this issue 2 years ago • 6 comments

Made some minor optimizations to udivmod4. Speed is not noticeably improved, but panic statements may be more helpful and binaries may be smaller.

NCGThompson avatar Oct 28 '23 03:10 NCGThompson

Previously, if the high() of both operands were zero and the rem return was set to Some, then each of the remainder and the quotient were calculated separately without any optimizations. % gives a different zero divisor message than /. Now, the function will only panic with /'s message, and the remainder is derived from the quotient. Note that even if udivmod4()'s caller checks for zero, the compiler likely won't eliminate the redundancy.

div_mod_knuth() expects the high high() of the denominator to be non-zero, and that holds true when called by udivmod4(), While div_mod_knuth() is public, its only caller in the library is udivmod4(). Despite being inlined, there were still some redundant checks when called through the function. When called directly, it relied on debug_asserts to validate the input. Now, the input is checked with a regular assert that has a custom error message. Because the input is always checked, we can safely put compiler hints further down, eliminating the redundant checks. The assert statement is elided when inlined in udivmod4(), leaving no redundant checks.

Results may vary depending on compiler settings and targets.

NCGThompson avatar Oct 28 '23 04:10 NCGThompson

Converted to draft so I can add docs to Knuth.

NCGThompson avatar Oct 28 '23 04:10 NCGThompson

udivmod4 usually isn't inlined, but it is always behind a safe wrapper. If we really wanted to optimize the binary size, we could make its abort rather than unwrapping, or even mark the intrinsics as unsafe and like I did to div_mod_knuth. It probably doesn't make a big difference though.

NCGThompson avatar Oct 30 '23 04:10 NCGThompson

Hey, sorry I haven't had time to look at this yet. Will get around to reviewing it either this weekend or early next week.

nlordell avatar Nov 04 '23 17:11 nlordell

As it turns out, making the division function infallible can make a big difference iff the actual result of the division is unneeded. However, in that case, it sounds reasonable to put the responsibility on the programmer to remove the useless 256-bit division call.

NCGThompson avatar Nov 05 '23 15:11 NCGThompson

This is great. Will run the fuzzer on this to make sure nothing broke, then will merge.

nlordell avatar Nov 16 '23 19:11 nlordell