Skip to content
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

Div by zero #31

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Div by zero #31

wants to merge 5 commits into from

Conversation

NCGThompson
Copy link
Contributor

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

@NCGThompson
Copy link
Contributor Author

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 NCGThompson marked this pull request as draft October 28, 2023 04:26
@NCGThompson
Copy link
Contributor Author

Converted to draft so I can add docs to Knuth.

@NCGThompson NCGThompson marked this pull request as ready for review October 29, 2023 02:55
@NCGThompson NCGThompson marked this pull request as draft October 29, 2023 20:55
@NCGThompson
Copy link
Contributor Author

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 NCGThompson marked this pull request as ready for review October 30, 2023 04:31
@nlordell
Copy link
Owner

nlordell commented Nov 4, 2023

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.

@NCGThompson
Copy link
Contributor Author

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.

@nlordell
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants