umul128 portable fallback computes an incorrect high product
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 10.5k
- Forks
- 2.1k
- Avg merge
- 3d 11h
- Merged PRs (30d)
- 7
Description
Description
The portable fallback of umul128 in include/cutlass/uint128.h computes a wrong high product whenever either operand has nonzero bits above the low 32 bits of both halves' cross terms, which is the common case.
The function is compiled into device code by nvcc on hosts where CUTLASS_INT128_ARITHMETIC is not defined (e.g. any Windows nvcc build: _MSC_VER is only honored when __CUDA_ARCH__ is undefined), and on non-x86-64/aarch64 hosts. It is also what host builds use on compilers without the MSVC intrinsic.
Root cause
include/cutlass/uint128.h, lines 80-90:
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;
Two problems in the schoolbook decomposition:
r_hiadds the low 32 bits of the cross products (& mask) instead of the high 32 bits (>> 32). Those low bits were already consumed at bit position 32 viap_mid, so they are double counted in the wrong place.- The carry out of
p_mid(p_mid >> 32) is never added intor_hi.
Minimal repro
#include "cutlass/uint128.h"
#include <cstdio>
int main() {
uint64_t hi = 0;
uint64_t lo = umul128(0xFFFFFFFFull, 0x100000000ull, &hi);
printf("lo=%016llx hi=%016llx\n", (unsigned long long)lo, (unsigned long long)hi);
}
Exact product is 0x00000000FFFFFFFF00000000, so hi must be 0.
Actual output (g++ -std=c++17 -O2, include-only, CUDA 12.8.1 devel container):
lo=ffffffff00000000 hi=00000000ffffffff
A randomized differential against unsigned __int128 (2,000,000 uniformly random 64-bit pairs) shows the stock implementation returns a wrong high product for every single pair; a corrected formula matches all 2,000,000:
stock mismatches over 2000000 random pairs: 2000000
fixed-formula mismatches: 0
Suggested fix
const uint64_t mask = 0xFFFFFFFF;
uint64_t a_lo = multiplier & mask;
uint64_t a_hi = multiplier >> 32;
uint64_t b_lo = multiplicand & mask;
uint64_t b_hi = multiplicand >> 32;
uint64_t p_ll = a_lo * b_lo;
uint64_t p_lh = a_lo * b_hi;
uint64_t p_hl = a_hi * b_lo;
uint64_t p_hh = a_hi * b_hi;
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);
*high_product = r_hi;
return r_lo;
Verified with the randomized differential above (0 / 2,000,000 mismatches vs unsigned __int128).
Notes
- No current in-tree caller exercises
umul128oruint128_t::operator*(uint64_t)outside the header itself (FastDivmodU64only usesoperator/, whose portable path is correct given itshigh < divisorcontract), so this has gone unnoticed. It silently corrupts results for any external user of this public helper.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in include/cutlass/uint128.h at the portable umul128 fallback and review the minimal repro from the issue. Correct the high-product calculation, then compare its output with the expected product and unsigned __int128 using the reported randomized differential checks; done means zero mismatches across those checks.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 82/100