Repository navigation
Conversation
bnb_host_warp_size() queried device 0 once and reused the result for every device. Its own comment admitted this returns the wrong value on mixed RDNA+CDNA systems, and bitsandbytes-foundation#2059 did not touch it. That matters because the launch geometry in ops.cu comes from the runtime value while kQuantizeBlockwiseSmall derives NUM_QB from the compile-time BNB_WARP_SIZE. On a wave64 device reached from a wave32 device (or the reverse) the launched block size disagrees with the kernel's NUM_QB: qb_id runs past warp_reduce[]/smem_absmax_value[], absmax writes land in the neighbouring block's slots, and the grid no longer covers all input. Query hipDeviceAttributeWarpSize for the *current* device and fall back to 32 when the query fails, so a failed query degrades to a conservative launch instead of an out-of-range one.
The kernel derives NUM_QB from the compile-time BNB_WARP_SIZE, so on gfx9 the host side falling back to 32 halves num_qb and sizes the grid for half the work. That is the same out-of-range access this change set is meant to prevent, just moved into the error path. Use the constant on both sides. This also covers device indices past the cache cap, where the previous code returned 32 without querying at all.
This branch has not been deployed
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.
Follow-up to #2059.
That one fixed the compile-time
BNB_WARP_SIZEso all of gfx9 gets 64. The runtime side inops.cuis still off though:bnb_host_warp_size()asks forhipDeviceAttributeWarpSizeon device 0 once and caches it for the whole process. The comment directly above it already says that's wrong on mixed RDNA+CDNA systems, so this is just closing that out.It's not cosmetic, because the two sides have to agree.
kQuantizeBlockwiseSmallderivesNUM_QBfrom the compile-timeBNB_WARP_SIZE, andops.cuderivesnum_qb, the grid and the block size from the runtime value. When those disagree, the kernel'sNUM_QBno longer matches how the launch was sized, and everything indexed byqb_idgoes with it:warp_reduce[]andsmem_absmax_value[]areNUM_QBelements, and the write isabsmax[blockIdx.x * NUM_QB + qb_id].It breaks differently depending on the direction:
NUM_QB = 1so a block covers 64 values, but the host sizes the grid for 2 quantization blocks per block, so the grid is half of what it needs to be and half the input never gets quantized.qb_idreaches 1 against one-element arrays.NUM_QB = 2so a block covers 128 values, but the grid is sized for 1 per block, so it walks twice as far as it should and theabsmaxwrites run past the buffer.Fix is to query the current device and cache per device, capped at 16. A failed query falls back to the compile-time
BNB_WARP_SIZErather than to 32, because the kernel'sNUM_QBis derived from that same constant, so it is the only value guaranteed to agree with the launch. On gfx9 that constant is 64, and falling back to 32 there would have reproduced the mismatch above instead of avoiding it.Checked on an RX 9070 XT (gfx1201, ROCm 7.2, clang 23, MSVC 18), building this branch on its own:
-DCOMPUTE_BACKEND=hip -DBNB_ROCM_ARCH=gfx1201builds cleantest_ops.py,test_linear4bit.py,test_linear8bitlt.py,test_autograd.py: 1342 passed, 149 skipped, 30 xfailed-DBNB_ROCM_ARCH=gfx906cross-compiles clean. It did not before [ROCm] Restore Wave64 warp size for all gfx9 targets #2059: at3fe95b5it fails instantiatingBlockLoad<__half, 32, 2, BLOCK_LOAD_WARP_TRANSPOSE>inkQuantizeBlockwise, with rocPRIM'sstatic assertion failed ... 'BlockSize % ::rocprim::arch::wavefront::min_size() == 0': BlockSize must be a multiple of hardware warpsize. That'sTHREADS=32pickingBLOCK_LOAD_WARP_TRANSPOSEon a 64-wide wave, so the gfx906 build break was real and [ROCm] Restore Wave64 warp size for all gfx9 targets #2059 closed it. This PR just keeps the host side consistent with that.What I could not check is the mixed wave32/wave64 case itself. I don't have a machine with both, and on a single-device system this returns the same value as before, so it's a no-op there. That's also why there's no new test: reproducing it needs two devices with different wave sizes, and I'd rather not add a test that only runs on hardware neither of us has.
One choice in here worth a second opinion: the 16-device cap is arbitrary. Past the cap the compile-time width is used, which is right for the arch the library was built for but wrong if someone genuinely has 17+ devices with mixed wave sizes, so a mutex-guarded vector is fine too if you'd rather not have a ceiling. That's easy to change.