-
Notifications
You must be signed in to change notification settings - Fork 147
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
[_TestSupport] Fix DoubleWidth think-o that trips an assert #273
Conversation
I updated dem issue with more of dem details:
Current status after this PR:
|
@LiarPrincess Thank you for providing more detail in the bug report regarding related edge cases! |
@swift-ci test |
Added Current status:
EDIT: Ooo… Interesting! In the original issue I added the "hint" (the |
@LiarPrincess, have you provided all of the failing test cases in the bug report? It would be good to see all of them in one go instead of iteratively expanding the bug. |
In the issue I mentioned:
To run those tests on
typealias UInt128 = DoubleWidth<UInt64>
typealias UInt256 = DoubleWidth<UInt128>
typealias Word = UInt64
EDIT: I created gists, so you can just copy-paste them (note that the 2nd one is quite big): |
@LiarPrincess, that's a lot of tests! Can you specifically list the failing ones in the bug report? |
You can just run them: copy the code from the gist and run it. They should finish execution in ~1 second. If there is a failure then Xcode will point you to an exact line. If there is a crash then it will automatically jump into a debugger.
In the issue I added 2 examples: test_d and test_e. I hope that resolving them will make all tests pass, but since I have not debugged your code it may be possible that there is some other bug hiding in there.
|
I also tested Anyway, I confirm that this PR fully solves #272. EDIT: CI says that if failed on Linux, but I tested it on both mac and Ubuntu, so this may be something related to CI, not to this PR. IDK. |
d28476f
to
a7e27e1
Compare
Great, cleaned up the commit history; this is ready to go. Will update main apple/swift repo soon. |
@swift-ci test |
1 similar comment
@swift-ci test |
Linux test failure is a misconfiguration of CI, which I've notified Mishal of. We'll merge this as-is. |
If
lhs
andrhs
have the same number of leading zero bits (and we knowrhs < lhs
), then we can't call_divide
to divide a triple-width magnitude by a double-width magnitude (this trips the assert on line 634), but rather we know that the quotient is one. Additionally, we must use>>
instead of&>>
when computing the high word because in this scenario the leading zero bits may be zero.Since it turns out there's more than one calling site for
_divide
that doesn't ensure the invariant asserted on line 634, we should actually deal with this in one place by replacing the assertion with aguard
that handles the special case.Additionally, fix a third think-o related to full-width multiplication where a carry isn't accounted for, causing unintended overflow with certain inputs.
This type now gives correct output verified for addition/subtraction/multiplication/nonzero-division-with-overflow for every pair of 16-bit unsigned integers using
DoubleWidth<DoubleWidth<UInt4>>
(with a customUInt4
implementation).Resolves #272