From f81f40749bb7d65558d2bc2860f0b0a653a36f68 Mon Sep 17 00:00:00 2001 From: Alex Duforest Date: Mon, 12 Oct 2026 14:54:19 -0700 Subject: [PATCH 1/6] test(rccl): add host microtests for nccl_device/utility.h math helpers --- projects/rccl/test/host/CMakeLists.txt | 1 + projects/rccl/test/host/MICROTEST_README.md | 5 + projects/rccl/test/host/nccl-utility-test.cc | 379 +++++++++++++++++++ 3 files changed, 385 insertions(+) create mode 100644 projects/rccl/test/host/nccl-utility-test.cc diff --git a/projects/rccl/test/host/CMakeLists.txt b/projects/rccl/test/host/CMakeLists.txt index 26a486a70cd9..e1d32bb1ec2a 100644 --- a/projects/rccl/test/host/CMakeLists.txt +++ b/projects/rccl/test/host/CMakeLists.txt @@ -97,6 +97,7 @@ function(rccl_define_micro_source_lists) devcomm-test.cc wrap-test.cc dev-runtime-test.cc + nccl-utility-test.cc # src/include/nccl_device/utility.h (header-only, no fakes) # sym-kernels-index-test.cc: added conditionally below, only when GENERATE_SYM_KERNELS is ON. gdr-probe-test.cc # src/misc/gdr_probe.cc runtime GDR registration probe process-isolated-runner-test.cc diff --git a/projects/rccl/test/host/MICROTEST_README.md b/projects/rccl/test/host/MICROTEST_README.md index a1df556582cc..087e9cdfbfbf 100644 --- a/projects/rccl/test/host/MICROTEST_README.md +++ b/projects/rccl/test/host/MICROTEST_README.md @@ -80,6 +80,11 @@ export colliding non-`static` symbols; otherwise a unit needs its own binary: - `dev_runtime.cc` (`DEV_RUNTIME_CC_PATH`, from `dev-runtime-test.cc`); suites `Alloc*`, `Comm*`, `Compute*`, `DeepCopy*`, `Dev*`, `Gin*`, `Nccl*`, `Sym*`, and `Win*`. + - `src/include/nccl_device/utility.h` (header-only, from + `nccl-utility-test.cc`); suite `NcclUtilityMicrotest.*` (also caught by the + `Nccl*` pattern above). No `_CC_PATH` macro needed since it's a header, + not a `.cc` production TU, and no fakes since every targeted function is + `NCCL_HOST_DEVICE_INLINE` pure host/device math with no HIP dependency. - the generated `sym_kernels_host.cc` (`SYM_KERNELS_HOST_CC_PATH`, from `sym-kernels-index-test.cc`); suites `SymKernelIndex*`, `SymAllEmittedCombinations/*`, `SymUnhandledCombinations/*` (covered by the diff --git a/projects/rccl/test/host/nccl-utility-test.cc b/projects/rccl/test/host/nccl-utility-test.cc new file mode 100644 index 000000000000..4c494456503a --- /dev/null +++ b/projects/rccl/test/host/nccl-utility-test.cc @@ -0,0 +1,379 @@ +/************************************************************************* + * Copyright (c) 2026 Advanced Micro Devices, Inc. All rights reserved. + * + * See LICENSE.txt for license information + ************************************************************************/ + +// Host-only microtests for the pure host/device math helpers in +// src/include/nccl_device/utility.h: rounding/alignment arithmetic, the +// fast-division reciprocal family, the memory_order conversion helpers, and +// the Optional/Present variadic construction helpers. All targeted +// functions are NCCL_HOST_DEVICE_INLINE with no HIP/device-only dependency, +// so the real header compiles and runs unmodified in a host-only TU -- no +// fakes needed. + +#include + +#include +#include +#include + +#include "nccl_device/utility.h" + +using nccl::utility::acquireOrderOf; +using nccl::utility::add4G; +using nccl::utility::alignDown; +using nccl::utility::alignUp; +using nccl::utility::divUp; +using nccl::utility::idivFast32; +using nccl::utility::idivFast64; +using nccl::utility::idivmodFast32; +using nccl::utility::idivmodFast64; +using nccl::utility::idivRcp32; +using nccl::utility::idivRcp64; +using nccl::utility::imodFast32; +using nccl::utility::imodFast64; +using nccl::utility::imulRcp32; +using nccl::utility::imulRcp64; +using nccl::utility::isPow2; +using nccl::utility::mul32hi; +using nccl::utility::mul64hi; +using nccl::utility::Optional; +using nccl::utility::present; +using nccl::utility::Present; +using nccl::utility::releaseOrderOf; +using nccl::utility::roundDown; +using nccl::utility::roundUp; +using nccl::utility::rollingLessEq; +using nccl::utility::rollingLessThan; +using nccl::utility::toAtomicBuiltinOrder; +using nccl::utility::toCudaOrder; + +namespace { + +// Brute-force reference: idivFast32/64 claim idivFast(x, y, idivRcp(y)) == x/y +// for every valid (x, y); these helpers recompute that division the ordinary +// way so the fast path can't share a bug with its own golden. +uint32_t refDiv32(uint32_t x, uint32_t y) { return x / y; } +uint64_t refDiv64(uint64_t x, uint64_t y) { return x / y; } + +} // namespace + +TEST(NcclUtilityMicrotest, DivUp_RoundsAwayFromZeroOnlyWhenRemainderIsNonzero) { + EXPECT_EQ(divUp(0u, 4u), 0u); + EXPECT_EQ(divUp(1u, 4u), 1u); + EXPECT_EQ(divUp(4u, 4u), 1u); + EXPECT_EQ(divUp(5u, 4u), 2u); + EXPECT_EQ(divUp(7u, 3u), 3u); +} + +TEST(NcclUtilityMicrotest, RoundUpDown_AreInverseDirectionsAroundAMultiple) { + EXPECT_EQ(roundUp(0u, 8u), 0u); + EXPECT_EQ(roundUp(1u, 8u), 8u); + EXPECT_EQ(roundUp(8u, 8u), 8u); + EXPECT_EQ(roundUp(9u, 8u), 16u); + + EXPECT_EQ(roundDown(0u, 8u), 0u); + EXPECT_EQ(roundDown(7u, 8u), 0u); + EXPECT_EQ(roundDown(8u, 8u), 8u); + EXPECT_EQ(roundDown(15u, 8u), 8u); +} + +TEST(NcclUtilityMicrotest, AlignUpDown_ScalarOverload_AssumesPowerOfTwoAlignment) { + EXPECT_EQ(alignUp(0u, 16u), 0u); + EXPECT_EQ(alignUp(1u, 16u), 16u); + EXPECT_EQ(alignUp(16u, 16u), 16u); + EXPECT_EQ(alignUp(17u, 16u), 32u); + + EXPECT_EQ(alignDown(0u, 16u), 0u); + EXPECT_EQ(alignDown(15u, 16u), 0u); + EXPECT_EQ(alignDown(16u, 16u), 16u); + EXPECT_EQ(alignDown(31u, 16u), 16u); +} + +TEST(NcclUtilityMicrotest, AlignUpDown_PointerOverload_RoundsTheAddressNotTheByte) { + alignas(32) char buf[64]; + // Single-byte-type overload: static_assert(sizeof(T) == 1) gates this at + // compile time, so the test itself is part of the contract being checked. + char* base = buf + 3; + char* up = alignUp(base, size_t(16)); + char* down = alignDown(base, size_t(16)); + EXPECT_EQ(reinterpret_cast(up) % 16, 0u); + EXPECT_EQ(reinterpret_cast(down) % 16, 0u); + EXPECT_GE(up, base); + EXPECT_LE(down, base); + EXPECT_LT(reinterpret_cast(up) - reinterpret_cast(down), 32u); +} + +TEST(NcclUtilityMicrotest, IsPow2_TrueOnlyForPowersOfTwoAndZero) { + EXPECT_TRUE(isPow2(0u)); + EXPECT_TRUE(isPow2(1u)); + EXPECT_TRUE(isPow2(2u)); + EXPECT_TRUE(isPow2(1024u)); + EXPECT_FALSE(isPow2(3u)); + EXPECT_FALSE(isPow2(6u)); + EXPECT_FALSE(isPow2(1023u)); +} + +TEST(NcclUtilityMicrotest, RollingLessEqAndLessThan_WrapAroundHalfTheRangeCorrectly) { + // 8-bit rolling counter: 250 is "less than" 4 (wrapped), but not vice versa. + EXPECT_TRUE(rollingLessEq(uint8_t(250), uint8_t(4), 8)); + EXPECT_FALSE(rollingLessEq(uint8_t(4), uint8_t(250), 8)); + EXPECT_TRUE(rollingLessThan(uint8_t(250), uint8_t(4), 8)); + EXPECT_FALSE(rollingLessThan(uint8_t(4), uint8_t(250), 8)); + + // Equal values: LessEq true, LessThan false (strict). + EXPECT_TRUE(rollingLessEq(uint8_t(7), uint8_t(7), 8)); + EXPECT_FALSE(rollingLessThan(uint8_t(7), uint8_t(7), 8)); + + // rollingLessThan is defined as !rollingLessEq(b, a); verify that identity + // directly so a copy-paste swap of a/b in either function is caught. + for (uint8_t a : {uint8_t(0), uint8_t(1), uint8_t(127), uint8_t(128), uint8_t(200)}) { + for (uint8_t b : {uint8_t(0), uint8_t(1), uint8_t(127), uint8_t(128), uint8_t(200)}) { + EXPECT_EQ(rollingLessThan(a, b, 8), !rollingLessEq(b, a, 8)) << "a=" << int(a) << " b=" << int(b); + } + } +} + +TEST(NcclUtilityMicrotest, IdivRcp32_MatchesExactFormulaIncludingPow2Correction) { + // Formula: uint32_t(-1)/x + isPow2(x). The +1 correction for powers of two + // is what makes idivFast32 exact when y is a power of two; drop it and the + // round-trip test below would start failing for y in {1,2,4,...}. + EXPECT_EQ(idivRcp32(1u), uint32_t(-1) / 1u + 1u); + EXPECT_EQ(idivRcp32(2u), uint32_t(-1) / 2u + 1u); + EXPECT_EQ(idivRcp32(3u), uint32_t(-1) / 3u + 0u); + EXPECT_EQ(idivRcp32(100u), uint32_t(-1) / 100u + 0u); +} + +TEST(NcclUtilityMicrotest, IdivRcp64_MatchesExactFormulaIncludingPow2Correction) { + EXPECT_EQ(idivRcp64(1ull), uint64_t(-1) / 1ull + 1ull); + EXPECT_EQ(idivRcp64(2ull), uint64_t(-1) / 2ull + 1ull); + EXPECT_EQ(idivRcp64(3ull), uint64_t(-1) / 3ull + 0ull); + EXPECT_EQ(idivRcp64(1ull << 40), uint64_t(-1) / (1ull << 40) + 1ull); +} + +TEST(NcclUtilityMicrotest, Mul32Hi_ReturnsTheHighHalfOfTheFull64BitProduct) { + EXPECT_EQ(mul32hi(0u, 0xFFFFFFFFu), 0u); + EXPECT_EQ(mul32hi(0xFFFFFFFFu, 0xFFFFFFFFu), uint32_t((uint64_t(0xFFFFFFFFu) * 0xFFFFFFFFu) >> 32)); + EXPECT_EQ(mul32hi(0x10000u, 0x10000u), 1u); // 2^16 * 2^16 == 2^32, high half == 1 +} + +TEST(NcclUtilityMicrotest, Mul64Hi_ReturnsTheHighHalfOfTheFull128BitProduct) { + EXPECT_EQ(mul64hi(0ull, ~0ull), 0ull); + uint64_t big = 0x1'0000'0000ull; // 2^32 + EXPECT_EQ(mul64hi(big, big), 1ull); // 2^32 * 2^32 == 2^64, high half == 1 +} + +TEST(NcclUtilityMicrotest, ImulRcp32_HandlesZeroReciprocalShortCircuitsAndMatchesIdivRcpOfProduct) { + // xrcp == 0 or yrcp == 0 short-circuits to the other operand's reciprocal + // directly -- callers use this to mean "treat this factor as already 1". + EXPECT_EQ(imulRcp32(5u, 0u, 7u, idivRcp32(7u)), idivRcp32(7u)); + EXPECT_EQ(imulRcp32(5u, idivRcp32(5u), 7u, 0u), idivRcp32(5u)); + + // x=3, y=5 is chosen (rather than some arbitrary pair) because it's one of + // the smallest inputs that actually drives the `if (x*y <= rem) rcp += 1;` + // rounding correction true -- an arbitrary pair like (6,7) can land on the + // false side and leave that correction untested. + uint32_t x = 3, y = 5; + uint32_t got = imulRcp32(x, idivRcp32(x), y, idivRcp32(y)); + EXPECT_EQ(got, idivRcp32(x * y)); + + // x=2, y=2 exercises the other side of that same branch (no correction + // needed), so both directions of the rounding check are verified. + EXPECT_EQ(imulRcp32(2u, idivRcp32(2u), 2u, idivRcp32(2u)), idivRcp32(4u)); +} + +TEST(NcclUtilityMicrotest, ImulRcp64_HandlesZeroReciprocalShortCircuitsAndMatchesIdivRcpOfProduct) { + EXPECT_EQ(imulRcp64(5ull, 0ull, 7ull, idivRcp64(7ull)), idivRcp64(7ull)); + EXPECT_EQ(imulRcp64(5ull, idivRcp64(5ull), 7ull, 0ull), idivRcp64(5ull)); + + // Same rationale as the 32-bit test above: (3,5) drives the rounding + // correction branch true, which a value like (6,7) does not for the 64-bit + // reciprocal (the correction's trigger condition depends on word width). + uint64_t x = 3, y = 5; + uint64_t got = imulRcp64(x, idivRcp64(x), y, idivRcp64(y)); + EXPECT_EQ(got, idivRcp64(x * y)); + + // (2,2) exercises the other side of that same branch (no correction needed). + EXPECT_EQ(imulRcp64(2ull, idivRcp64(2ull), 2ull, idivRcp64(2ull)), idivRcp64(4ull)); +} + +TEST(NcclUtilityMicrotest, IdivmodFast32_MatchesOrdinaryDivisionAcrossDivisorsAndYrcpZeroShortcut) { + for (uint32_t y : {1u, 2u, 3u, 7u, 16u, 100u, 4096u}) { + uint32_t yrcp = idivRcp32(y); + for (uint32_t x : {0u, 1u, 5u, 41u, 4095u, 4096u, 1000000u}) { + uint32_t q, r; + idivmodFast32(&q, &r, x, y, yrcp); + EXPECT_EQ(q, x / y) << "x=" << x << " y=" << y; + EXPECT_EQ(r, x % y) << "x=" << x << " y=" << y; + } + } + // yrcp == 0 shortcut: q = x directly (caller's contract for "divisor is 1"). + uint32_t q, r; + idivmodFast32(&q, &r, 42u, 1u, 0u); + EXPECT_EQ(q, 42u); + EXPECT_EQ(r, 0u); +} + +TEST(NcclUtilityMicrotest, IdivmodFast64_MatchesOrdinaryDivisionAcrossDivisorsAndYrcpZeroShortcut) { + for (uint64_t y : {1ull, 2ull, 3ull, 7ull, 16ull, 100ull, 1ull << 20}) { + uint64_t yrcp = idivRcp64(y); + for (uint64_t x : {0ull, 1ull, 5ull, 41ull, (1ull << 20) - 1, 1ull << 20, 1ull << 40}) { + uint64_t q, r; + idivmodFast64(&q, &r, x, y, yrcp); + EXPECT_EQ(q, x / y) << "x=" << x << " y=" << y; + EXPECT_EQ(r, x % y) << "x=" << x << " y=" << y; + } + } + uint64_t q, r; + idivmodFast64(&q, &r, 42ull, 1ull, 0ull); + EXPECT_EQ(q, 42ull); + EXPECT_EQ(r, 0ull); +} + +TEST(NcclUtilityMicrotest, IdivFast32_MatchesRefDivisionAndIdivFast64TruncatesToLow32Bits) { + for (uint32_t y : {1u, 3u, 16u, 4096u}) { + uint32_t yrcp = idivRcp32(y); + for (uint32_t x : {0u, 41u, 4095u, 1000000u}) { + EXPECT_EQ(idivFast32(x, y, yrcp), refDiv32(x, y)); + } + } + // idivFast64 returns a uint32_t truncation of a 64-bit quotient; pick a + // quotient that fits in 32 bits so truncation is a correctness check, not + // a free pass. + uint64_t y = 1000, yrcp64 = idivRcp64(y); + uint64_t x = 123456789ull; + EXPECT_EQ(idivFast64(x, y, yrcp64), uint32_t(refDiv64(x, y))); +} + +TEST(NcclUtilityMicrotest, ImodFast32And64_MatchRefModulo) { + for (uint32_t y : {1u, 3u, 16u, 4096u}) { + uint32_t yrcp = idivRcp32(y); + for (uint32_t x : {0u, 41u, 4095u, 1000000u}) { + EXPECT_EQ(imodFast32(x, y, yrcp), x % y); + } + } + for (uint64_t y : {1ull, 3ull, 16ull, 1ull << 20}) { + uint64_t yrcp = idivRcp64(y); + for (uint64_t x : {0ull, 41ull, (1ull << 20) - 1, 1ull << 40}) { + EXPECT_EQ(imodFast64(x, y, yrcp), x % y); + } + } +} + +TEST(NcclUtilityMicrotest, Optional_DefaultAndAbsentConstructLeavePresentFalse) { + Optional a; + EXPECT_FALSE(a.present); + Optional b{nccl::utility::Absent{}}; + EXPECT_FALSE(b.present); +} + +TEST(NcclUtilityMicrotest, Optional_PresentConstructorForwardsArgsAndSetsPresentTrue) { + // present(1, 2) packs two ints into a Present; Optional's + // Present-taking constructor must forward both through to T's constructor + // in the right order, not just construct a default T. + struct Pair { + int a, b; + NCCL_HOST_DEVICE_INLINE Pair(int a_, int b_) : a(a_), b(b_) {} + }; + Optional opt(present(3, 4)); + ASSERT_TRUE(opt.present); + EXPECT_EQ(opt.thing.a, 3); + EXPECT_EQ(opt.thing.b, 4); +} + +TEST(NcclUtilityMicrotest, Optional_DestructorOnlyRunsWhenPresentIsTrue) { + static int destructorCalls = 0; + struct Counted { + NCCL_HOST_DEVICE_INLINE Counted() {} + NCCL_HOST_DEVICE_INLINE ~Counted() { destructorCalls++; } + }; + + destructorCalls = 0; + { + Optional absent; // present == false: ~Counted() must NOT run. + } + EXPECT_EQ(destructorCalls, 0); + + destructorCalls = 0; + { + Optional present_opt(present()); + } + EXPECT_EQ(destructorCalls, 1); +} + +TEST(NcclUtilityMicrotest, Add4G_AddsDeltaIntoTheHighHalfOfA64BitValueViaUnion) { + // add4G reinterprets a T (must be 8 bytes) as two uint32_t halves and + // adds delta4G into the high half (index 1 is the high word on this + // little-endian build). Verify against plain 64-bit arithmetic. + uint64_t base = 0x0000'0000'1234'5678ull; + EXPECT_EQ(add4G(base, 1), base + (uint64_t(1) << 32)); + EXPECT_EQ(add4G(base, 5), base + (uint64_t(5) << 32)); + // Negative delta: subtracts a 4G chunk from the high half. + uint64_t highBase = 0x0000'0003'0000'0000ull; + EXPECT_EQ(add4G(highBase, -1), highBase - (uint64_t(1) << 32)); +} + +TEST(NcclUtilityMicrotest, AcquireOrderOfStd_DowngradesReleaseAndAcqRelPassesEverythingElseThrough) { + EXPECT_EQ(acquireOrderOf(std::memory_order_release), std::memory_order_relaxed); + EXPECT_EQ(acquireOrderOf(std::memory_order_acq_rel), std::memory_order_acquire); + EXPECT_EQ(acquireOrderOf(std::memory_order_relaxed), std::memory_order_relaxed); + EXPECT_EQ(acquireOrderOf(std::memory_order_acquire), std::memory_order_acquire); + EXPECT_EQ(acquireOrderOf(std::memory_order_seq_cst), std::memory_order_seq_cst); +} + +TEST(NcclUtilityMicrotest, ReleaseOrderOfStd_DowngradesAcquireAndAcqRelPassesEverythingElseThrough) { + EXPECT_EQ(releaseOrderOf(std::memory_order_acquire), std::memory_order_relaxed); + EXPECT_EQ(releaseOrderOf(std::memory_order_acq_rel), std::memory_order_release); + EXPECT_EQ(releaseOrderOf(std::memory_order_relaxed), std::memory_order_relaxed); + EXPECT_EQ(releaseOrderOf(std::memory_order_release), std::memory_order_release); + EXPECT_EQ(releaseOrderOf(std::memory_order_seq_cst), std::memory_order_seq_cst); +} + +TEST(NcclUtilityMicrotest, ToAtomicBuiltinOrder_MapsEveryStdOrderAndFallsBackToSeqCstForConsume) { + EXPECT_EQ(toAtomicBuiltinOrder(std::memory_order_relaxed), __ATOMIC_RELAXED); + EXPECT_EQ(toAtomicBuiltinOrder(std::memory_order_acquire), __ATOMIC_ACQUIRE); + EXPECT_EQ(toAtomicBuiltinOrder(std::memory_order_release), __ATOMIC_RELEASE); + EXPECT_EQ(toAtomicBuiltinOrder(std::memory_order_acq_rel), __ATOMIC_ACQ_REL); + EXPECT_EQ(toAtomicBuiltinOrder(std::memory_order_seq_cst), __ATOMIC_SEQ_CST); + // memory_order_consume has no case label; must fall through to default. + EXPECT_EQ(toAtomicBuiltinOrder(std::memory_order_consume), __ATOMIC_SEQ_CST); +} + +TEST(NcclUtilityMicrotest, AcquireOrderOfCuda_DowngradesReleaseAndAcqRelPassesEverythingElseThrough) { + EXPECT_EQ(acquireOrderOf(cuda::memory_order_release), cuda::memory_order_relaxed); + EXPECT_EQ(acquireOrderOf(cuda::memory_order_acq_rel), cuda::memory_order_acquire); + EXPECT_EQ(acquireOrderOf(cuda::memory_order_relaxed), cuda::memory_order_relaxed); + EXPECT_EQ(acquireOrderOf(cuda::memory_order_acquire), cuda::memory_order_acquire); + EXPECT_EQ(acquireOrderOf(cuda::memory_order_seq_cst), cuda::memory_order_seq_cst); +} + +TEST(NcclUtilityMicrotest, ReleaseOrderOfCuda_DowngradesAcquireAndAcqRelPassesEverythingElseThrough) { + EXPECT_EQ(releaseOrderOf(cuda::memory_order_acquire), cuda::memory_order_relaxed); + EXPECT_EQ(releaseOrderOf(cuda::memory_order_acq_rel), cuda::memory_order_release); + EXPECT_EQ(releaseOrderOf(cuda::memory_order_relaxed), cuda::memory_order_relaxed); + EXPECT_EQ(releaseOrderOf(cuda::memory_order_release), cuda::memory_order_release); + EXPECT_EQ(releaseOrderOf(cuda::memory_order_seq_cst), cuda::memory_order_seq_cst); +} + +TEST(NcclUtilityMicrotest, ToCudaOrder_MapsEveryStdOrderAndFallsBackToSeqCstForConsume) { + EXPECT_EQ(toCudaOrder(std::memory_order_relaxed), cuda::memory_order_relaxed); + EXPECT_EQ(toCudaOrder(std::memory_order_acquire), cuda::memory_order_acquire); + EXPECT_EQ(toCudaOrder(std::memory_order_release), cuda::memory_order_release); + EXPECT_EQ(toCudaOrder(std::memory_order_acq_rel), cuda::memory_order_acq_rel); + EXPECT_EQ(toCudaOrder(std::memory_order_seq_cst), cuda::memory_order_seq_cst); + EXPECT_EQ(toCudaOrder(std::memory_order_consume), cuda::memory_order_seq_cst); +} + +TEST(NcclUtilityMicrotest, Present_GetByIndexReturnsEachArgumentInDeclarationOrder) { + // Pass lvalues so H deduces to int& (present(H&&, ...) with an lvalue arg + // collapses to a reference to the original), keeping p's stored references + // valid for the lifetime of a,b,c below -- a prvalue-literal argument would + // deduce H=int and leave p holding a dangling int&& into a temporary that + // expires at the end of the present(...) call's full expression. + int a = 10, b = 20, c = 30; + auto p = present(a, b, c); + EXPECT_EQ(p.get(nccl::utility::IntSeq<0>{}), 10); + EXPECT_EQ(p.get(nccl::utility::IntSeq<1>{}), 20); + EXPECT_EQ(p.get(nccl::utility::IntSeq<2>{}), 30); +} From 067754716a7e7789013b5a91bd95590337552882 Mon Sep 17 00:00:00 2001 From: Alex Duforest Date: Mon, 12 Oct 2026 22:25:43 -0700 Subject: [PATCH 2/6] test(rccl): address ARGUS findings on utility.h microtests - rollingLessEq: add a partial-width (nBits=32 on a uint64_t) pair; every existing call passed nBits == full width, leaving the mask shift dead. - idivFast64: add a quotient above 2^32 (1<<40 / 1); the existing case's quotient fit in 32 bits, so the truncation it claimed to test was inert. - AlignUp/AlignDown pointer overload: pin exact addresses instead of mod/bound checks that an off-by-one in the production formula also passes. - roundUp/roundDown: add a non-power-of-two modulus pair (3), since modulus-8-only can't tell % from a power-of-two mask. - idivRcp32/64 sweeps: add 0xFFFFFFFFu / ~0ull to catch an over-estimating reciprocal, which the existing +1 under-estimate correction can't. - Remove a tautological loop in the rolling-comparator test: it reduced to the same call on both sides regardless of a swap bug, so it could not fail. - Fix two comments: the IdivRcp32 pow2-correction is a branch-avoidance optimisation, not required for round-trip correctness; and the 'no HIP/device-only dependency' header/README claim doesn't hold for the four memory_order helpers (true only because this build's --offload-host-only path still invokes hipcc). - Two self-referential assertions (mul32hi/mul64hi) replaced with literal expected values. - present_opt -> presentOpt naming nit. --- projects/rccl/test/host/MICROTEST_README.md | 7 +- projects/rccl/test/host/nccl-utility-test.cc | 74 +++++++++++++------- 2 files changed, 52 insertions(+), 29 deletions(-) diff --git a/projects/rccl/test/host/MICROTEST_README.md b/projects/rccl/test/host/MICROTEST_README.md index 087e9cdfbfbf..45c0ea2e12fc 100644 --- a/projects/rccl/test/host/MICROTEST_README.md +++ b/projects/rccl/test/host/MICROTEST_README.md @@ -83,8 +83,11 @@ export colliding non-`static` symbols; otherwise a unit needs its own binary: - `src/include/nccl_device/utility.h` (header-only, from `nccl-utility-test.cc`); suite `NcclUtilityMicrotest.*` (also caught by the `Nccl*` pattern above). No `_CC_PATH` macro needed since it's a header, - not a `.cc` production TU, and no fakes since every targeted function is - `NCCL_HOST_DEVICE_INLINE` pure host/device math with no HIP dependency. + not a `.cc` production TU, and no fakes since every targeted function + compiles cleanly in a host-only TU -- most are `NCCL_HOST_DEVICE_INLINE` + pure math with no HIP dependency; the four `memory_order` conversion + helpers sit inside `#if NCCL_CHECK_CUDACC`, which this build satisfies + because this target's `--offload-host-only` build still invokes hipcc. - the generated `sym_kernels_host.cc` (`SYM_KERNELS_HOST_CC_PATH`, from `sym-kernels-index-test.cc`); suites `SymKernelIndex*`, `SymAllEmittedCombinations/*`, `SymUnhandledCombinations/*` (covered by the diff --git a/projects/rccl/test/host/nccl-utility-test.cc b/projects/rccl/test/host/nccl-utility-test.cc index 4c494456503a..e5b233d17073 100644 --- a/projects/rccl/test/host/nccl-utility-test.cc +++ b/projects/rccl/test/host/nccl-utility-test.cc @@ -7,16 +7,18 @@ // Host-only microtests for the pure host/device math helpers in // src/include/nccl_device/utility.h: rounding/alignment arithmetic, the // fast-division reciprocal family, the memory_order conversion helpers, and -// the Optional/Present variadic construction helpers. All targeted -// functions are NCCL_HOST_DEVICE_INLINE with no HIP/device-only dependency, -// so the real header compiles and runs unmodified in a host-only TU -- no -// fakes needed. +// the Optional/Present variadic construction helpers. Most targeted +// functions are NCCL_HOST_DEVICE_INLINE with no HIP/device-only dependency; +// acquireOrderOf/releaseOrderOf/toAtomicBuiltinOrder/toCudaOrder are the +// exception -- they sit inside `#if NCCL_CHECK_CUDACC`, which this build +// satisfies because both the real device build and this host-only +// `--offload-host-only` build still invoke hipcc. The real header compiles +// and runs unmodified in a host-only TU either way -- no fakes needed. #include #include #include -#include #include "nccl_device/utility.h" @@ -77,6 +79,11 @@ TEST(NcclUtilityMicrotest, RoundUpDown_AreInverseDirectionsAroundAMultiple) { EXPECT_EQ(roundDown(7u, 8u), 0u); EXPECT_EQ(roundDown(8u, 8u), 8u); EXPECT_EQ(roundDown(15u, 8u), 8u); + + // Non-power-of-two modulus: these use % y, not a power-of-two mask. Without + // this pair, modulus 8 alone can't tell `% y` apart from `& (y - 1)`. + EXPECT_EQ(roundUp(5u, 3u), 6u); + EXPECT_EQ(roundDown(7u, 3u), 6u); } TEST(NcclUtilityMicrotest, AlignUpDown_ScalarOverload_AssumesPowerOfTwoAlignment) { @@ -95,14 +102,15 @@ TEST(NcclUtilityMicrotest, AlignUpDown_PointerOverload_RoundsTheAddressNotTheByt alignas(32) char buf[64]; // Single-byte-type overload: static_assert(sizeof(T) == 1) gates this at // compile time, so the test itself is part of the contract being checked. + // Exact expectations, not just "divisible by 16": the mod/bound checks + // below would equally pass an off-by-one mutation of utility.h's + // `(addr + a - 1) & -uintptr_t(a)` to `(addr + a) & -uintptr_t(a)`, since + // buf + 3 rounds to buf + 16 either way. char* base = buf + 3; char* up = alignUp(base, size_t(16)); char* down = alignDown(base, size_t(16)); - EXPECT_EQ(reinterpret_cast(up) % 16, 0u); - EXPECT_EQ(reinterpret_cast(down) % 16, 0u); - EXPECT_GE(up, base); - EXPECT_LE(down, base); - EXPECT_LT(reinterpret_cast(up) - reinterpret_cast(down), 32u); + EXPECT_EQ(up, buf + 16); + EXPECT_EQ(down, buf); } TEST(NcclUtilityMicrotest, IsPow2_TrueOnlyForPowersOfTwoAndZero) { @@ -126,19 +134,22 @@ TEST(NcclUtilityMicrotest, RollingLessEqAndLessThan_WrapAroundHalfTheRangeCorrec EXPECT_TRUE(rollingLessEq(uint8_t(7), uint8_t(7), 8)); EXPECT_FALSE(rollingLessThan(uint8_t(7), uint8_t(7), 8)); - // rollingLessThan is defined as !rollingLessEq(b, a); verify that identity - // directly so a copy-paste swap of a/b in either function is caught. - for (uint8_t a : {uint8_t(0), uint8_t(1), uint8_t(127), uint8_t(128), uint8_t(200)}) { - for (uint8_t b : {uint8_t(0), uint8_t(1), uint8_t(127), uint8_t(128), uint8_t(200)}) { - EXPECT_EQ(rollingLessThan(a, b, 8), !rollingLessEq(b, a, 8)) << "a=" << int(a) << " b=" << int(b); - } - } + // Every call above passes nBits == 8*sizeof(Uint) (a no-op mask), so the + // `m = Uint(-1) >> (8*sizeof(Uint) - nBits)` shift at utility.h:197 is + // never actually exercised -- production calls it with a narrower runtime + // width (gin_barrier__funcs.h:110 passes 32 for a uint64_t counter). Use a + // wider type with an explicit partial-width nBits so the mask is load-bearing: + // true only because the high 32 bits are masked out of the wraparound check. + EXPECT_TRUE(rollingLessEq(uint64_t(0xFFFFFFF0ull), uint64_t(0x10ull), 32)); + EXPECT_FALSE(rollingLessEq(uint64_t(0x10ull), uint64_t(0xFFFFFFF0ull), 32)); } TEST(NcclUtilityMicrotest, IdivRcp32_MatchesExactFormulaIncludingPow2Correction) { - // Formula: uint32_t(-1)/x + isPow2(x). The +1 correction for powers of two - // is what makes idivFast32 exact when y is a power of two; drop it and the - // round-trip test below would start failing for y in {1,2,4,...}. + // Formula: uint32_t(-1)/x + isPow2(x). The +1 term is a branch-avoidance + // optimisation, not a correctness requirement: idivFast32's own + // `if (r >= y) { q += 1; r -= y; }` correction step already recovers the + // right answer even without it. These assertions pin the exact reciprocal + // value the formula produces, not just that the round-trip still works. EXPECT_EQ(idivRcp32(1u), uint32_t(-1) / 1u + 1u); EXPECT_EQ(idivRcp32(2u), uint32_t(-1) / 2u + 1u); EXPECT_EQ(idivRcp32(3u), uint32_t(-1) / 3u + 0u); @@ -154,7 +165,7 @@ TEST(NcclUtilityMicrotest, IdivRcp64_MatchesExactFormulaIncludingPow2Correction) TEST(NcclUtilityMicrotest, Mul32Hi_ReturnsTheHighHalfOfTheFull64BitProduct) { EXPECT_EQ(mul32hi(0u, 0xFFFFFFFFu), 0u); - EXPECT_EQ(mul32hi(0xFFFFFFFFu, 0xFFFFFFFFu), uint32_t((uint64_t(0xFFFFFFFFu) * 0xFFFFFFFFu) >> 32)); + EXPECT_EQ(mul32hi(0xFFFFFFFFu, 0xFFFFFFFFu), 0xFFFFFFFEu); EXPECT_EQ(mul32hi(0x10000u, 0x10000u), 1u); // 2^16 * 2^16 == 2^32, high half == 1 } @@ -162,6 +173,7 @@ TEST(NcclUtilityMicrotest, Mul64Hi_ReturnsTheHighHalfOfTheFull128BitProduct) { EXPECT_EQ(mul64hi(0ull, ~0ull), 0ull); uint64_t big = 0x1'0000'0000ull; // 2^32 EXPECT_EQ(mul64hi(big, big), 1ull); // 2^32 * 2^32 == 2^64, high half == 1 + EXPECT_EQ(mul64hi(~0ull, ~0ull), 0xFFFFFFFFFFFFFFFEull); } TEST(NcclUtilityMicrotest, ImulRcp32_HandlesZeroReciprocalShortCircuitsAndMatchesIdivRcpOfProduct) { @@ -201,7 +213,12 @@ TEST(NcclUtilityMicrotest, ImulRcp64_HandlesZeroReciprocalShortCircuitsAndMatche TEST(NcclUtilityMicrotest, IdivmodFast32_MatchesOrdinaryDivisionAcrossDivisorsAndYrcpZeroShortcut) { for (uint32_t y : {1u, 2u, 3u, 7u, 16u, 100u, 4096u}) { uint32_t yrcp = idivRcp32(y); - for (uint32_t x : {0u, 1u, 5u, 41u, 4095u, 4096u, 1000000u}) { + // 0xFFFFFFFFu exercises the top of the range: idivRcp32's single + // `if (r >= y)` correction step only recovers an *under*-estimating + // reciprocal, so an over-estimate (e.g. an erroneous unconditional +1) + // is only caught near the dividend's upper bound, not by the smaller + // values below. + for (uint32_t x : {0u, 1u, 5u, 41u, 4095u, 4096u, 1000000u, 0xFFFFFFFFu}) { uint32_t q, r; idivmodFast32(&q, &r, x, y, yrcp); EXPECT_EQ(q, x / y) << "x=" << x << " y=" << y; @@ -218,7 +235,8 @@ TEST(NcclUtilityMicrotest, IdivmodFast32_MatchesOrdinaryDivisionAcrossDivisorsAn TEST(NcclUtilityMicrotest, IdivmodFast64_MatchesOrdinaryDivisionAcrossDivisorsAndYrcpZeroShortcut) { for (uint64_t y : {1ull, 2ull, 3ull, 7ull, 16ull, 100ull, 1ull << 20}) { uint64_t yrcp = idivRcp64(y); - for (uint64_t x : {0ull, 1ull, 5ull, 41ull, (1ull << 20) - 1, 1ull << 20, 1ull << 40}) { + // ~0ull is the 64-bit counterpart of the 32-bit sweep's 0xFFFFFFFFu above. + for (uint64_t x : {0ull, 1ull, 5ull, 41ull, (1ull << 20) - 1, 1ull << 20, 1ull << 40, ~0ull}) { uint64_t q, r; idivmodFast64(&q, &r, x, y, yrcp); EXPECT_EQ(q, x / y) << "x=" << x << " y=" << y; @@ -238,12 +256,14 @@ TEST(NcclUtilityMicrotest, IdivFast32_MatchesRefDivisionAndIdivFast64TruncatesTo EXPECT_EQ(idivFast32(x, y, yrcp), refDiv32(x, y)); } } - // idivFast64 returns a uint32_t truncation of a 64-bit quotient; pick a - // quotient that fits in 32 bits so truncation is a correctness check, not - // a free pass. + // idivFast64 narrows a 64-bit quotient to uint32_t. A quotient that fits in + // 32 bits leaves the narrowing inert (the oracle's own uint32_t cast is a + // no-op too), so the second case below -- whose true quotient is far above + // 2^32 -- is the one that actually exercises the truncation. uint64_t y = 1000, yrcp64 = idivRcp64(y); uint64_t x = 123456789ull; EXPECT_EQ(idivFast64(x, y, yrcp64), uint32_t(refDiv64(x, y))); + EXPECT_EQ(idivFast64(1ull << 40, 1ull, idivRcp64(1ull)), 0u); } TEST(NcclUtilityMicrotest, ImodFast32And64_MatchRefModulo) { @@ -297,7 +317,7 @@ TEST(NcclUtilityMicrotest, Optional_DestructorOnlyRunsWhenPresentIsTrue) { destructorCalls = 0; { - Optional present_opt(present()); + Optional presentOpt(present()); } EXPECT_EQ(destructorCalls, 1); } From 47a12553f3b0487d753d89fcb75aa2e1e52dade1 Mon Sep 17 00:00:00 2001 From: Alex Duforest Date: Tue, 13 Oct 2026 12:05:46 -0700 Subject: [PATCH 3/6] test(rccl): pin alignUp's - 1, rollingLessThan's nBits forward, trim wrapper sweeps Co-Authored-By: Claude --- projects/rccl/test/host/nccl-utility-test.cc | 50 +++++++++----------- 1 file changed, 22 insertions(+), 28 deletions(-) diff --git a/projects/rccl/test/host/nccl-utility-test.cc b/projects/rccl/test/host/nccl-utility-test.cc index e5b233d17073..10a83346b076 100644 --- a/projects/rccl/test/host/nccl-utility-test.cc +++ b/projects/rccl/test/host/nccl-utility-test.cc @@ -53,10 +53,8 @@ using nccl::utility::toCudaOrder; namespace { -// Brute-force reference: idivFast32/64 claim idivFast(x, y, idivRcp(y)) == x/y -// for every valid (x, y); these helpers recompute that division the ordinary -// way so the fast path can't share a bug with its own golden. -uint32_t refDiv32(uint32_t x, uint32_t y) { return x / y; } +// Reference for idivFast64's truncation check: recomputes the division the +// ordinary way so the fast path can't share a bug with its own golden. uint64_t refDiv64(uint64_t x, uint64_t y) { return x / y; } } // namespace @@ -102,15 +100,17 @@ TEST(NcclUtilityMicrotest, AlignUpDown_PointerOverload_RoundsTheAddressNotTheByt alignas(32) char buf[64]; // Single-byte-type overload: static_assert(sizeof(T) == 1) gates this at // compile time, so the test itself is part of the contract being checked. - // Exact expectations, not just "divisible by 16": the mod/bound checks - // below would equally pass an off-by-one mutation of utility.h's - // `(addr + a - 1) & -uintptr_t(a)` to `(addr + a) & -uintptr_t(a)`, since - // buf + 3 rounds to buf + 16 either way. + // buf + 3 rounds to buf + 16 under both `(addr + a - 1) & -uintptr_t(a)` and + // an off-by-one `(addr + a) & -uintptr_t(a)`, so the unaligned case alone + // cannot pin the `- 1`. An already-aligned pointer can: correct returns buf, + // the mutant returns buf + 16. alignDown has no `- 1` term, so only alignUp + // needs the extra case. char* base = buf + 3; char* up = alignUp(base, size_t(16)); char* down = alignDown(base, size_t(16)); EXPECT_EQ(up, buf + 16); EXPECT_EQ(down, buf); + EXPECT_EQ(alignUp(buf, size_t(16)), buf); } TEST(NcclUtilityMicrotest, IsPow2_TrueOnlyForPowersOfTwoAndZero) { @@ -142,6 +142,11 @@ TEST(NcclUtilityMicrotest, RollingLessEqAndLessThan_WrapAroundHalfTheRangeCorrec // true only because the high 32 bits are masked out of the wraparound check. EXPECT_TRUE(rollingLessEq(uint64_t(0xFFFFFFF0ull), uint64_t(0x10ull), 32)); EXPECT_FALSE(rollingLessEq(uint64_t(0x10ull), uint64_t(0xFFFFFFF0ull), 32)); + + // rollingLessThan must forward nBits rather than fall back to its own + // default: dropping the forward at utility.h:202 flips both of these. + EXPECT_TRUE(rollingLessThan(uint64_t(0xFFFFFFF0ull), uint64_t(0x10ull), 32)); + EXPECT_FALSE(rollingLessThan(uint64_t(0x10ull), uint64_t(0xFFFFFFF0ull), 32)); } TEST(NcclUtilityMicrotest, IdivRcp32_MatchesExactFormulaIncludingPow2Correction) { @@ -249,13 +254,10 @@ TEST(NcclUtilityMicrotest, IdivmodFast64_MatchesOrdinaryDivisionAcrossDivisorsAn EXPECT_EQ(r, 0ull); } -TEST(NcclUtilityMicrotest, IdivFast32_MatchesRefDivisionAndIdivFast64TruncatesToLow32Bits) { - for (uint32_t y : {1u, 3u, 16u, 4096u}) { - uint32_t yrcp = idivRcp32(y); - for (uint32_t x : {0u, 41u, 4095u, 1000000u}) { - EXPECT_EQ(idivFast32(x, y, yrcp), refDiv32(x, y)); - } - } +TEST(NcclUtilityMicrotest, IdivFast32_ReturnsTheQuotientAndIdivFast64TruncatesToLow32Bits) { + // idivFast32 is a one-line wrapper over idivmodFast32, which the sweep above + // already checks; one case pins that it returns q (13), not r (2). + EXPECT_EQ(idivFast32(41u, 3u, idivRcp32(3u)), 13u); // idivFast64 narrows a 64-bit quotient to uint32_t. A quotient that fits in // 32 bits leaves the narrowing inert (the oracle's own uint32_t cast is a // no-op too), so the second case below -- whose true quotient is far above @@ -266,19 +268,11 @@ TEST(NcclUtilityMicrotest, IdivFast32_MatchesRefDivisionAndIdivFast64TruncatesTo EXPECT_EQ(idivFast64(1ull << 40, 1ull, idivRcp64(1ull)), 0u); } -TEST(NcclUtilityMicrotest, ImodFast32And64_MatchRefModulo) { - for (uint32_t y : {1u, 3u, 16u, 4096u}) { - uint32_t yrcp = idivRcp32(y); - for (uint32_t x : {0u, 41u, 4095u, 1000000u}) { - EXPECT_EQ(imodFast32(x, y, yrcp), x % y); - } - } - for (uint64_t y : {1ull, 3ull, 16ull, 1ull << 20}) { - uint64_t yrcp = idivRcp64(y); - for (uint64_t x : {0ull, 41ull, (1ull << 20) - 1, 1ull << 40}) { - EXPECT_EQ(imodFast64(x, y, yrcp), x % y); - } - } +TEST(NcclUtilityMicrotest, ImodFast32And64_ReturnTheRemainder) { + // imodFast32/64 are one-line wrappers over idivmodFast32/64, which the sweeps + // above already check; one case each pins that they return r (2), not q (13). + EXPECT_EQ(imodFast32(41u, 3u, idivRcp32(3u)), 2u); + EXPECT_EQ(imodFast64(41ull, 3ull, idivRcp64(3ull)), 2ull); } TEST(NcclUtilityMicrotest, Optional_DefaultAndAbsentConstructLeavePresentFalse) { From d1eb14cf6b99b8689fa05795f159356cbdcab412 Mon Sep 17 00:00:00 2001 From: Alex Duforest Date: Tue, 13 Oct 2026 13:53:40 -0700 Subject: [PATCH 4/6] test(rccl): correct the imulRcp32 counterexample comment, note the bitops.h using-declaration trap Co-Authored-By: Claude --- projects/rccl/test/host/nccl-utility-test.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/projects/rccl/test/host/nccl-utility-test.cc b/projects/rccl/test/host/nccl-utility-test.cc index 10a83346b076..36107fbde779 100644 --- a/projects/rccl/test/host/nccl-utility-test.cc +++ b/projects/rccl/test/host/nccl-utility-test.cc @@ -22,6 +22,9 @@ #include "nccl_device/utility.h" +// bitops.h declares same-signature ::divUp, ::isPow2, ::add4G, ::idivRcp32 and more, so these +// global-scope using-declarations become ill-formed if it ever reaches this TU; qualify the calls +// instead if a new include pulls it in (test/BitOpsTests.cpp records the same trap for add4G). using nccl::utility::acquireOrderOf; using nccl::utility::add4G; using nccl::utility::alignDown; @@ -189,8 +192,8 @@ TEST(NcclUtilityMicrotest, ImulRcp32_HandlesZeroReciprocalShortCircuitsAndMatche // x=3, y=5 is chosen (rather than some arbitrary pair) because it's one of // the smallest inputs that actually drives the `if (x*y <= rem) rcp += 1;` - // rounding correction true -- an arbitrary pair like (6,7) can land on the - // false side and leave that correction untested. + // rounding correction true: rcp comes back as 0x11111110, rem == 16, and + // 15 <= 16. Pairs like the (2,2) case below land on the false side instead. uint32_t x = 3, y = 5; uint32_t got = imulRcp32(x, idivRcp32(x), y, idivRcp32(y)); EXPECT_EQ(got, idivRcp32(x * y)); From 87d73103f23f9cfe1cbb3832b575af40608abf32 Mon Sep 17 00:00:00 2001 From: Alex Duforest Date: Tue, 13 Oct 2026 19:56:47 -0700 Subject: [PATCH 5/6] test(rccl): pin alignDown's negation, rollingLessEq's half-range boundary, and imulRcp's rem negation Co-Authored-By: Claude --- projects/rccl/test/host/nccl-utility-test.cc | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/projects/rccl/test/host/nccl-utility-test.cc b/projects/rccl/test/host/nccl-utility-test.cc index 36107fbde779..d3302371351d 100644 --- a/projects/rccl/test/host/nccl-utility-test.cc +++ b/projects/rccl/test/host/nccl-utility-test.cc @@ -97,6 +97,9 @@ TEST(NcclUtilityMicrotest, AlignUpDown_ScalarOverload_AssumesPowerOfTwoAlignment EXPECT_EQ(alignDown(15u, 16u), 0u); EXPECT_EQ(alignDown(16u, 16u), 16u); EXPECT_EQ(alignDown(31u, 16u), 16u); + // Above 2*a: below it, `x & -Z(a)` (utility.h:166) cannot be told from `x & Z(a)`. + EXPECT_EQ(alignDown(48u, 16u), 48u); + EXPECT_EQ(alignDown(33u, 16u), 32u); } TEST(NcclUtilityMicrotest, AlignUpDown_PointerOverload_RoundsTheAddressNotTheByte) { @@ -137,6 +140,11 @@ TEST(NcclUtilityMicrotest, RollingLessEqAndLessThan_WrapAroundHalfTheRangeCorrec EXPECT_TRUE(rollingLessEq(uint8_t(7), uint8_t(7), 8)); EXPECT_FALSE(rollingLessThan(uint8_t(7), uint8_t(7), 8)); + // The half-range boundary itself: m is 0xFF here, so m >> 1 is 127. A + // distance of exactly 127 is still "less or equal", 128 is not. + EXPECT_TRUE(rollingLessEq(uint8_t(0), uint8_t(127), 8)); + EXPECT_FALSE(rollingLessEq(uint8_t(0), uint8_t(128), 8)); + // Every call above passes nBits == 8*sizeof(Uint) (a no-op mask), so the // `m = Uint(-1) >> (8*sizeof(Uint) - nBits)` shift at utility.h:197 is // never actually exercised -- production calls it with a narrower runtime @@ -201,6 +209,11 @@ TEST(NcclUtilityMicrotest, ImulRcp32_HandlesZeroReciprocalShortCircuitsAndMatche // x=2, y=2 exercises the other side of that same branch (no correction // needed), so both directions of the rounding check are verified. EXPECT_EQ(imulRcp32(2u, idivRcp32(2u), 2u, idivRcp32(2u)), idivRcp32(4u)); + + // x=3, y=3 pins the `0u -` in rem: mul32hi already returns the exact + // 0x1C71C71C, so rem is 4 and 9 <= 4 wants no correction. Without the + // negation rem is 0xFFFFFFFC and the correction fires, returning one too many. + EXPECT_EQ(imulRcp32(3u, idivRcp32(3u), 3u, idivRcp32(3u)), idivRcp32(9u)); } TEST(NcclUtilityMicrotest, ImulRcp64_HandlesZeroReciprocalShortCircuitsAndMatchesIdivRcpOfProduct) { @@ -216,6 +229,10 @@ TEST(NcclUtilityMicrotest, ImulRcp64_HandlesZeroReciprocalShortCircuitsAndMatche // (2,2) exercises the other side of that same branch (no correction needed). EXPECT_EQ(imulRcp64(2ull, idivRcp64(2ull), 2ull, idivRcp64(2ull)), idivRcp64(4ull)); + + // (3,3) pins the `0 -` in rem the same way: mul64hi returns the exact + // 0x1C71C71C71C71C71, so rem is 7 and 9 <= 7 wants no correction. + EXPECT_EQ(imulRcp64(3ull, idivRcp64(3ull), 3ull, idivRcp64(3ull)), idivRcp64(9ull)); } TEST(NcclUtilityMicrotest, IdivmodFast32_MatchesOrdinaryDivisionAcrossDivisorsAndYrcpZeroShortcut) { From f41e1d5ee67a35ce6efbb965ea333ec47f63e466 Mon Sep 17 00:00:00 2001 From: Alex Duforest Date: Wed, 14 Oct 2026 15:40:03 -0700 Subject: [PATCH 6/6] test(rccl): trim microtest docs and comments to match the other host tests Shorten the MICROTEST_README entry, CMake comments and test-file comments to the length the other host microtests use. Comment and doc changes only. Co-Authored-By: Claude --- projects/rccl/test/host/MICROTEST_README.md | 10 +- projects/rccl/test/host/nccl-utility-test.cc | 108 +++++-------------- 2 files changed, 27 insertions(+), 91 deletions(-) diff --git a/projects/rccl/test/host/MICROTEST_README.md b/projects/rccl/test/host/MICROTEST_README.md index 45c0ea2e12fc..88215181402e 100644 --- a/projects/rccl/test/host/MICROTEST_README.md +++ b/projects/rccl/test/host/MICROTEST_README.md @@ -80,14 +80,8 @@ export colliding non-`static` symbols; otherwise a unit needs its own binary: - `dev_runtime.cc` (`DEV_RUNTIME_CC_PATH`, from `dev-runtime-test.cc`); suites `Alloc*`, `Comm*`, `Compute*`, `DeepCopy*`, `Dev*`, `Gin*`, `Nccl*`, `Sym*`, and `Win*`. - - `src/include/nccl_device/utility.h` (header-only, from - `nccl-utility-test.cc`); suite `NcclUtilityMicrotest.*` (also caught by the - `Nccl*` pattern above). No `_CC_PATH` macro needed since it's a header, - not a `.cc` production TU, and no fakes since every targeted function - compiles cleanly in a host-only TU -- most are `NCCL_HOST_DEVICE_INLINE` - pure math with no HIP dependency; the four `memory_order` conversion - helpers sit inside `#if NCCL_CHECK_CUDACC`, which this build satisfies - because this target's `--offload-host-only` build still invokes hipcc. + - `include/nccl_device/utility.h` (header-only, from `nccl-utility-test.cc`); + suite `NcclUtilityMicrotest.*`. - the generated `sym_kernels_host.cc` (`SYM_KERNELS_HOST_CC_PATH`, from `sym-kernels-index-test.cc`); suites `SymKernelIndex*`, `SymAllEmittedCombinations/*`, `SymUnhandledCombinations/*` (covered by the diff --git a/projects/rccl/test/host/nccl-utility-test.cc b/projects/rccl/test/host/nccl-utility-test.cc index d3302371351d..716c707db603 100644 --- a/projects/rccl/test/host/nccl-utility-test.cc +++ b/projects/rccl/test/host/nccl-utility-test.cc @@ -4,16 +4,7 @@ * See LICENSE.txt for license information ************************************************************************/ -// Host-only microtests for the pure host/device math helpers in -// src/include/nccl_device/utility.h: rounding/alignment arithmetic, the -// fast-division reciprocal family, the memory_order conversion helpers, and -// the Optional/Present variadic construction helpers. Most targeted -// functions are NCCL_HOST_DEVICE_INLINE with no HIP/device-only dependency; -// acquireOrderOf/releaseOrderOf/toAtomicBuiltinOrder/toCudaOrder are the -// exception -- they sit inside `#if NCCL_CHECK_CUDACC`, which this build -// satisfies because both the real device build and this host-only -// `--offload-host-only` build still invoke hipcc. The real header compiles -// and runs unmodified in a host-only TU either way -- no fakes needed. +// Host-only microtests for the host/device math helpers in src/include/nccl_device/utility.h. #include @@ -22,9 +13,8 @@ #include "nccl_device/utility.h" -// bitops.h declares same-signature ::divUp, ::isPow2, ::add4G, ::idivRcp32 and more, so these -// global-scope using-declarations become ill-formed if it ever reaches this TU; qualify the calls -// instead if a new include pulls it in (test/BitOpsTests.cpp records the same trap for add4G). +// bitops.h declares the same ::divUp, ::isPow2, ... so these using-declarations break if it is +// ever included here; qualify the calls instead. using nccl::utility::acquireOrderOf; using nccl::utility::add4G; using nccl::utility::alignDown; @@ -56,8 +46,7 @@ using nccl::utility::toCudaOrder; namespace { -// Reference for idivFast64's truncation check: recomputes the division the -// ordinary way so the fast path can't share a bug with its own golden. +// Plain division, so the fast path can't share a bug with its golden. uint64_t refDiv64(uint64_t x, uint64_t y) { return x / y; } } // namespace @@ -81,8 +70,7 @@ TEST(NcclUtilityMicrotest, RoundUpDown_AreInverseDirectionsAroundAMultiple) { EXPECT_EQ(roundDown(8u, 8u), 8u); EXPECT_EQ(roundDown(15u, 8u), 8u); - // Non-power-of-two modulus: these use % y, not a power-of-two mask. Without - // this pair, modulus 8 alone can't tell `% y` apart from `& (y - 1)`. + // Non-power-of-two modulus, so `% y` can't pass as `& (y - 1)`. EXPECT_EQ(roundUp(5u, 3u), 6u); EXPECT_EQ(roundDown(7u, 3u), 6u); } @@ -97,20 +85,14 @@ TEST(NcclUtilityMicrotest, AlignUpDown_ScalarOverload_AssumesPowerOfTwoAlignment EXPECT_EQ(alignDown(15u, 16u), 0u); EXPECT_EQ(alignDown(16u, 16u), 16u); EXPECT_EQ(alignDown(31u, 16u), 16u); - // Above 2*a: below it, `x & -Z(a)` (utility.h:166) cannot be told from `x & Z(a)`. + // Above 2*a, where `x & -Z(a)` differs from `x & Z(a)`. EXPECT_EQ(alignDown(48u, 16u), 48u); EXPECT_EQ(alignDown(33u, 16u), 32u); } TEST(NcclUtilityMicrotest, AlignUpDown_PointerOverload_RoundsTheAddressNotTheByte) { alignas(32) char buf[64]; - // Single-byte-type overload: static_assert(sizeof(T) == 1) gates this at - // compile time, so the test itself is part of the contract being checked. - // buf + 3 rounds to buf + 16 under both `(addr + a - 1) & -uintptr_t(a)` and - // an off-by-one `(addr + a) & -uintptr_t(a)`, so the unaligned case alone - // cannot pin the `- 1`. An already-aligned pointer can: correct returns buf, - // the mutant returns buf + 16. alignDown has no `- 1` term, so only alignUp - // needs the extra case. + // An already-aligned pointer pins alignUp's `- 1`: buf + 3 rounds to buf + 16 either way. char* base = buf + 3; char* up = alignUp(base, size_t(16)); char* down = alignDown(base, size_t(16)); @@ -140,32 +122,22 @@ TEST(NcclUtilityMicrotest, RollingLessEqAndLessThan_WrapAroundHalfTheRangeCorrec EXPECT_TRUE(rollingLessEq(uint8_t(7), uint8_t(7), 8)); EXPECT_FALSE(rollingLessThan(uint8_t(7), uint8_t(7), 8)); - // The half-range boundary itself: m is 0xFF here, so m >> 1 is 127. A - // distance of exactly 127 is still "less or equal", 128 is not. + // The half-range boundary: 127 is still "less or equal", 128 is not. EXPECT_TRUE(rollingLessEq(uint8_t(0), uint8_t(127), 8)); EXPECT_FALSE(rollingLessEq(uint8_t(0), uint8_t(128), 8)); - // Every call above passes nBits == 8*sizeof(Uint) (a no-op mask), so the - // `m = Uint(-1) >> (8*sizeof(Uint) - nBits)` shift at utility.h:197 is - // never actually exercised -- production calls it with a narrower runtime - // width (gin_barrier__funcs.h:110 passes 32 for a uint64_t counter). Use a - // wider type with an explicit partial-width nBits so the mask is load-bearing: - // true only because the high 32 bits are masked out of the wraparound check. + // A partial nBits on a wider type, as production uses it (gin_barrier__funcs.h passes 32 for a + // uint64_t counter): true only because the high 32 bits are masked out. EXPECT_TRUE(rollingLessEq(uint64_t(0xFFFFFFF0ull), uint64_t(0x10ull), 32)); EXPECT_FALSE(rollingLessEq(uint64_t(0x10ull), uint64_t(0xFFFFFFF0ull), 32)); - // rollingLessThan must forward nBits rather than fall back to its own - // default: dropping the forward at utility.h:202 flips both of these. + // rollingLessThan must forward nBits: dropping it flips both. EXPECT_TRUE(rollingLessThan(uint64_t(0xFFFFFFF0ull), uint64_t(0x10ull), 32)); EXPECT_FALSE(rollingLessThan(uint64_t(0x10ull), uint64_t(0xFFFFFFF0ull), 32)); } TEST(NcclUtilityMicrotest, IdivRcp32_MatchesExactFormulaIncludingPow2Correction) { - // Formula: uint32_t(-1)/x + isPow2(x). The +1 term is a branch-avoidance - // optimisation, not a correctness requirement: idivFast32's own - // `if (r >= y) { q += 1; r -= y; }` correction step already recovers the - // right answer even without it. These assertions pin the exact reciprocal - // value the formula produces, not just that the round-trip still works. + // uint32_t(-1)/x + isPow2(x), pinned exactly: idivFast32's correction would hide a wrong +1. EXPECT_EQ(idivRcp32(1u), uint32_t(-1) / 1u + 1u); EXPECT_EQ(idivRcp32(2u), uint32_t(-1) / 2u + 1u); EXPECT_EQ(idivRcp32(3u), uint32_t(-1) / 3u + 0u); @@ -193,26 +165,19 @@ TEST(NcclUtilityMicrotest, Mul64Hi_ReturnsTheHighHalfOfTheFull128BitProduct) { } TEST(NcclUtilityMicrotest, ImulRcp32_HandlesZeroReciprocalShortCircuitsAndMatchesIdivRcpOfProduct) { - // xrcp == 0 or yrcp == 0 short-circuits to the other operand's reciprocal - // directly -- callers use this to mean "treat this factor as already 1". + // A zero reciprocal means "this factor is 1", so the other one is returned. EXPECT_EQ(imulRcp32(5u, 0u, 7u, idivRcp32(7u)), idivRcp32(7u)); EXPECT_EQ(imulRcp32(5u, idivRcp32(5u), 7u, 0u), idivRcp32(5u)); - // x=3, y=5 is chosen (rather than some arbitrary pair) because it's one of - // the smallest inputs that actually drives the `if (x*y <= rem) rcp += 1;` - // rounding correction true: rcp comes back as 0x11111110, rem == 16, and - // 15 <= 16. Pairs like the (2,2) case below land on the false side instead. + // (3,5) takes the `x*y <= rem` rounding correction. uint32_t x = 3, y = 5; uint32_t got = imulRcp32(x, idivRcp32(x), y, idivRcp32(y)); EXPECT_EQ(got, idivRcp32(x * y)); - // x=2, y=2 exercises the other side of that same branch (no correction - // needed), so both directions of the rounding check are verified. + // (2,2) doesn't. EXPECT_EQ(imulRcp32(2u, idivRcp32(2u), 2u, idivRcp32(2u)), idivRcp32(4u)); - // x=3, y=3 pins the `0u -` in rem: mul32hi already returns the exact - // 0x1C71C71C, so rem is 4 and 9 <= 4 wants no correction. Without the - // negation rem is 0xFFFFFFFC and the correction fires, returning one too many. + // (3,3) pins the `0u -` in rem: without it the correction fires and returns one too many. EXPECT_EQ(imulRcp32(3u, idivRcp32(3u), 3u, idivRcp32(3u)), idivRcp32(9u)); } @@ -220,29 +185,20 @@ TEST(NcclUtilityMicrotest, ImulRcp64_HandlesZeroReciprocalShortCircuitsAndMatche EXPECT_EQ(imulRcp64(5ull, 0ull, 7ull, idivRcp64(7ull)), idivRcp64(7ull)); EXPECT_EQ(imulRcp64(5ull, idivRcp64(5ull), 7ull, 0ull), idivRcp64(5ull)); - // Same rationale as the 32-bit test above: (3,5) drives the rounding - // correction branch true, which a value like (6,7) does not for the 64-bit - // reciprocal (the correction's trigger condition depends on word width). + // Same three cases as the 32-bit version. uint64_t x = 3, y = 5; uint64_t got = imulRcp64(x, idivRcp64(x), y, idivRcp64(y)); EXPECT_EQ(got, idivRcp64(x * y)); - // (2,2) exercises the other side of that same branch (no correction needed). EXPECT_EQ(imulRcp64(2ull, idivRcp64(2ull), 2ull, idivRcp64(2ull)), idivRcp64(4ull)); - // (3,3) pins the `0 -` in rem the same way: mul64hi returns the exact - // 0x1C71C71C71C71C71, so rem is 7 and 9 <= 7 wants no correction. EXPECT_EQ(imulRcp64(3ull, idivRcp64(3ull), 3ull, idivRcp64(3ull)), idivRcp64(9ull)); } TEST(NcclUtilityMicrotest, IdivmodFast32_MatchesOrdinaryDivisionAcrossDivisorsAndYrcpZeroShortcut) { for (uint32_t y : {1u, 2u, 3u, 7u, 16u, 100u, 4096u}) { uint32_t yrcp = idivRcp32(y); - // 0xFFFFFFFFu exercises the top of the range: idivRcp32's single - // `if (r >= y)` correction step only recovers an *under*-estimating - // reciprocal, so an over-estimate (e.g. an erroneous unconditional +1) - // is only caught near the dividend's upper bound, not by the smaller - // values below. + // The top of the range catches an over-estimated reciprocal; smaller values don't. for (uint32_t x : {0u, 1u, 5u, 41u, 4095u, 4096u, 1000000u, 0xFFFFFFFFu}) { uint32_t q, r; idivmodFast32(&q, &r, x, y, yrcp); @@ -250,7 +206,7 @@ TEST(NcclUtilityMicrotest, IdivmodFast32_MatchesOrdinaryDivisionAcrossDivisorsAn EXPECT_EQ(r, x % y) << "x=" << x << " y=" << y; } } - // yrcp == 0 shortcut: q = x directly (caller's contract for "divisor is 1"). + // yrcp == 0: q = x ("divisor is 1"). uint32_t q, r; idivmodFast32(&q, &r, 42u, 1u, 0u); EXPECT_EQ(q, 42u); @@ -260,7 +216,6 @@ TEST(NcclUtilityMicrotest, IdivmodFast32_MatchesOrdinaryDivisionAcrossDivisorsAn TEST(NcclUtilityMicrotest, IdivmodFast64_MatchesOrdinaryDivisionAcrossDivisorsAndYrcpZeroShortcut) { for (uint64_t y : {1ull, 2ull, 3ull, 7ull, 16ull, 100ull, 1ull << 20}) { uint64_t yrcp = idivRcp64(y); - // ~0ull is the 64-bit counterpart of the 32-bit sweep's 0xFFFFFFFFu above. for (uint64_t x : {0ull, 1ull, 5ull, 41ull, (1ull << 20) - 1, 1ull << 20, 1ull << 40, ~0ull}) { uint64_t q, r; idivmodFast64(&q, &r, x, y, yrcp); @@ -275,13 +230,9 @@ TEST(NcclUtilityMicrotest, IdivmodFast64_MatchesOrdinaryDivisionAcrossDivisorsAn } TEST(NcclUtilityMicrotest, IdivFast32_ReturnsTheQuotientAndIdivFast64TruncatesToLow32Bits) { - // idivFast32 is a one-line wrapper over idivmodFast32, which the sweep above - // already checks; one case pins that it returns q (13), not r (2). + // Wrapper over idivmodFast32: returns q (13), not r (2). EXPECT_EQ(idivFast32(41u, 3u, idivRcp32(3u)), 13u); - // idivFast64 narrows a 64-bit quotient to uint32_t. A quotient that fits in - // 32 bits leaves the narrowing inert (the oracle's own uint32_t cast is a - // no-op too), so the second case below -- whose true quotient is far above - // 2^32 -- is the one that actually exercises the truncation. + // The second case's quotient is far above 2^32, so the narrowing to uint32_t matters. uint64_t y = 1000, yrcp64 = idivRcp64(y); uint64_t x = 123456789ull; EXPECT_EQ(idivFast64(x, y, yrcp64), uint32_t(refDiv64(x, y))); @@ -289,8 +240,7 @@ TEST(NcclUtilityMicrotest, IdivFast32_ReturnsTheQuotientAndIdivFast64TruncatesTo } TEST(NcclUtilityMicrotest, ImodFast32And64_ReturnTheRemainder) { - // imodFast32/64 are one-line wrappers over idivmodFast32/64, which the sweeps - // above already check; one case each pins that they return r (2), not q (13). + // Wrappers over idivmodFast32/64: return r (2), not q (13). EXPECT_EQ(imodFast32(41u, 3u, idivRcp32(3u)), 2u); EXPECT_EQ(imodFast64(41ull, 3ull, idivRcp64(3ull)), 2ull); } @@ -303,9 +253,7 @@ TEST(NcclUtilityMicrotest, Optional_DefaultAndAbsentConstructLeavePresentFalse) } TEST(NcclUtilityMicrotest, Optional_PresentConstructorForwardsArgsAndSetsPresentTrue) { - // present(1, 2) packs two ints into a Present; Optional's - // Present-taking constructor must forward both through to T's constructor - // in the right order, not just construct a default T. + // Both values must reach T's constructor, in order. struct Pair { int a, b; NCCL_HOST_DEVICE_INLINE Pair(int a_, int b_) : a(a_), b(b_) {} @@ -337,9 +285,7 @@ TEST(NcclUtilityMicrotest, Optional_DestructorOnlyRunsWhenPresentIsTrue) { } TEST(NcclUtilityMicrotest, Add4G_AddsDeltaIntoTheHighHalfOfA64BitValueViaUnion) { - // add4G reinterprets a T (must be 8 bytes) as two uint32_t halves and - // adds delta4G into the high half (index 1 is the high word on this - // little-endian build). Verify against plain 64-bit arithmetic. + // add4G adds delta4G into the high 32 bits; check against plain 64-bit arithmetic. uint64_t base = 0x0000'0000'1234'5678ull; EXPECT_EQ(add4G(base, 1), base + (uint64_t(1) << 32)); EXPECT_EQ(add4G(base, 5), base + (uint64_t(5) << 32)); @@ -400,11 +346,7 @@ TEST(NcclUtilityMicrotest, ToCudaOrder_MapsEveryStdOrderAndFallsBackToSeqCstForC } TEST(NcclUtilityMicrotest, Present_GetByIndexReturnsEachArgumentInDeclarationOrder) { - // Pass lvalues so H deduces to int& (present(H&&, ...) with an lvalue arg - // collapses to a reference to the original), keeping p's stored references - // valid for the lifetime of a,b,c below -- a prvalue-literal argument would - // deduce H=int and leave p holding a dangling int&& into a temporary that - // expires at the end of the present(...) call's full expression. + // Lvalues, so p holds references to a, b, c rather than to dangling temporaries. int a = 10, b = 20, c = 30; auto p = present(a, b, c); EXPECT_EQ(p.get(nccl::utility::IntSeq<0>{}), 10);