Fix incorrect high product in umul128 portable fallback - #3520
Open
VaggelisGian wants to merge 1 commit into
Open
Fix incorrect high product in umul128 portable fallback#3520VaggelisGian wants to merge 1 commit into
VaggelisGian wants to merge 1 commit into
Conversation
The schoolbook decomposition in umul128() added the low 32 bits of both
cross products into r_hi where their high 32 bits belong, and dropped
the carry out of the mid sum. The portable path is what nvcc compiles
into device code on Windows hosts and what non-x86/aarch64 hosts use,
so any caller of this public helper silently got a wrong high word
whenever cross terms were nonzero. No in-tree caller exercises it
today (FastDivmodU64 uses operator/ only), which is why this survived.
The mid/lo/hi tail now follows the standard identity: the mid sum
carries into both words instead of re-adding consumed low halves.
New tests in test/unit/core/uint128.cu compare umul128 against an
independent 16-bit-limb schoolbook reference over edge combinations
and deterministic random pairs, host and device.
Test Plan:
nvidia/cuda:12.8.1-devel container, nvcc V12.8.93, sm_120a, RTX 5060 Ti:
cmake -G Ninja -DCUTLASS_NVCC_ARCHS=120a -DCUTLASS_ENABLE_TESTS=ON ..
ninja cutlass_test_unit_core
./test/unit/core/cutlass_test_unit_core --gtest_filter='uint128_t.*'
[ PASSED ] 4 tests.
With only the header fix reverted, both new tests fail (host stream
mismatches on 4096/4096 pairs; edge grid on 36/64).
Standalone randomized differential vs unsigned __int128 over
2,000,000 pairs: old code wrong high word on all 2,000,000,
fixed code on none.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #3492
The portable fallback of
umul128()ininclude/cutlass/uint128.hcomputed an incorrect high product. In the schoolbook decomposition the code added the low 32 bits of both cross products (p_lh & mask,p_hl & mask) intor_hiwhere the high 32 bits (>> 32) belong, double-counting bits that were already consumed at bit position 32 via the mid sum, and it dropped the carry out of the mid sum entirely.Before / after:
Minimal repro before the fix:
umul128(0xFFFFFFFFull, 0x100000000ull, &hi)leaveshi = 0xFFFFFFFFalthough the exact product0x00000000FFFFFFFF00000000has a zero high word.Test plan
New tests in
test/unit/core/uint128.cu(host_umul128_high_product,device_umul128_high_product) compareumul128against an independent 16-bit-limb schoolbook reference over edge combinations (0, 1, 2, 0xFFFFFFFF, 0x100000000, 2^64-1, 2^63, 0x123456789ABCDEF0 crossed with each other) plus deterministic xorshift random pairs, host and device.Full run on the fixed code, nvidia/cuda:12.8.1-devel container, nvcc V12.8.93,
-DCUTLASS_NVCC_ARCHS=120a, RTX 5060 Ti (sm_120a):Fail-first evidence, header fix reverted with these tests present:
unsigned __int128over 2,000,000 uniformly random 64-bit pairs: old implementation wrong high word on all 2,000,000 pairs, fixed implementation on none.Notes
umul128today (FastDivmodU64usesoperator/only), so the fix changes no existing behavior.CUTLASS_INT128_ARITHMETICis host-MSVC-x64 only), plus gcc/clang hosts on x86-64/aarch64 where the native__int128path coversuint128_t::operator*but notumul128itself.