NVIDIA / NVIDIA/cutlass

umul128 portable fallback computes an incorrect high product

Open Beginner friendly
#3,492 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

CUTLASS C++
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:

  1. r_hi adds 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 via p_mid, so they are double counted in the wrong place.
  2. The carry out of p_mid (p_mid >> 32) is never added into r_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 umul128 or uint128_t::operator*(uint64_t) outside the header itself (FastDivmodU64 only uses operator/, whose portable path is correct given its high < divisor contract), 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

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.