Skip to content

Fix incorrect high product in umul128 portable fallback - #3520

Open
VaggelisGian wants to merge 1 commit into
NVIDIA:mainfrom
VaggelisGian:fix-umul128-portable-high
Open

Fix incorrect high product in umul128 portable fallback#3520
VaggelisGian wants to merge 1 commit into
NVIDIA:mainfrom
VaggelisGian:fix-umul128-portable-high

Conversation

@VaggelisGian

@VaggelisGian VaggelisGian commented Aug 24, 2026

Copy link
Copy Markdown

Summary

Fixes #3492

The portable fallback of umul128() in include/cutlass/uint128.h computed 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) into r_hi where 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:

// before
uint64_t p_mid = (p_ll >> 32) + (p_lh & mask) + (p_hl & mask);
uint64_t r_lo  = (p_ll & mask) + (p_mid << 32);
uint64_t r_hi  = (p_lh & mask) + (p_hl & mask) + p_hh;

// after
uint64_t mid  = (p_ll >> 32) + (p_lh & mask) + (p_hl & mask);
uint64_t r_lo = (p_ll & mask) | (mid << 32);
uint64_t r_hi = p_hh + (p_lh >> 32) + (p_hl >> 32) + (mid >> 32);

Minimal repro before the fix: umul128(0xFFFFFFFFull, 0x100000000ull, &hi) leaves hi = 0xFFFFFFFF although the exact product 0x00000000FFFFFFFF00000000 has a zero high word.

Test plan

New tests in test/unit/core/uint128.cu (host_umul128_high_product, device_umul128_high_product) compare umul128 against 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):

Note: Google Test filter = uint128_t.*
[==========] Running 4 tests from 1 test suite.
[----------] 4 tests from uint128_t
[ RUN      ] uint128_t.host_arithmetic
[       OK ] uint128_t.host_arithmetic (4 ms)
[ RUN      ] uint128_t.host_umul128_high_product
[       OK ] uint128_t.host_umul128_high_product (0 ms)
[ RUN      ] uint128_t.device_arithmetic
[       OK ] uint128_t.device_arithmetic (233 ms)
[ RUN      ] uint128_t.device_umul128_high_product
[       OK ] uint128_t.device_umul128_high_product (1 ms)
[----------] 4 tests from uint128_t (239 ms total)
[==========] 4 tests from 1 test suite ran. (239 ms total)
[  PASSED  ] 4 tests.

Fail-first evidence, header fix reverted with these tests present:

  • Replaying the exact host test input streams against the old formula: the random stream mismatches on 4096/4096 iterations and the edge grid on 36/64 pairs (smallest failing case: a=1, b=2^32, where hi must be 0 but the old code returns 1).
  • A standalone randomized differential against unsigned __int128 over 2,000,000 uniformly random 64-bit pairs: old implementation wrong high word on all 2,000,000 pairs, fixed implementation on none.

Notes

  • The pre-existing tests in this file only multiply values below 1024 and check the result truncated to 64 bits, which is why the defect was never caught.
  • No in-tree caller exercises umul128 today (FastDivmodU64 uses operator/ only), so the fix changes no existing behavior.
  • Scope note: this portable path is what every nvcc device compile uses on all host OSes (CUTLASS_INT128_ARITHMETIC is host-MSVC-x64 only), plus gcc/clang hosts on x86-64/aarch64 where the native __int128 path covers uint128_t::operator* but not umul128 itself.

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.
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.

umul128 portable fallback computes an incorrect high product

1 participant