Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions numba_cuda/numba/cuda/codegen.py
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,9 @@ def _ensure_cc(self, cc):
return cc

device = devices.get_context().device
return device.compute_capability
cc = device.compute_capability
cc = (cc[0], cc[1], "a" if cc >= (9, 0) else "")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoded logic for automatic arch suffix assignment: The code automatically assigns "a" suffix for all cc >= 9.0. However:

  1. Not all GPUs with cc >= 9.0 necessarily have an "a" variant
  2. The logic doesn't handle "f" variants (like sm_100f mentioned in tests)
  3. Users might want cc >= 9.0 without arch suffix

Consider either:

  • Making this configurable via an environment variable/config option
  • Or providing a way to explicitly opt-out of automatic arch suffix
  • Or documenting this behavior clearly so users understand that cc >= 9.0 automatically gets "a" suffix

return cc
Comment on lines +212 to +214

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks backward compatibility by modifying compute_capability from a 2-tuple to a 3-tuple. Throughout the codebase, device.compute_capability is expected to be a 2-tuple (major, minor) and is compared with 2-tuples:

  • testing.py:282: cc >= (major, minor) where cc = device.compute_capability
  • driver.py:591: self.compute_capability < MIN_REQUIRED_CC
  • driver.py:622: self.compute_capability >= (5, 3)
  • Multiple test files perform similar comparisons

After line 213 executes, cc becomes a 3-tuple, which will break tuple comparisons. For example, (9, 0, "a") >= (9, 0) evaluates to True in Python, but (8, 0, "") >= (9, 0) would incorrectly evaluate to False due to string comparison.

The modification should only happen locally within this method and not affect the device's compute_capability property. Consider storing the arch-specific version separately or ensure all comparison sites are updated to handle 3-tuples.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The device's compute capability is not being affected.


def get_asm_str(self, cc=None):
return "\n".join(self.get_asm_strs(cc=cc))
Expand Down Expand Up @@ -347,7 +349,7 @@ def get_cufunc(self):
cufunc = self._cufunc_cache.get(device.id, None)
if cufunc:
return cufunc
cubin = self.get_cubin(cc=device.compute_capability)
cubin = self.get_cubin()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of the explicit cc=device.compute_capability parameter may cause issues. The get_cubin() method will call _ensure_cc(None), which will fetch device.compute_capability and convert it to a 3-tuple with the arch suffix.

However, if the device's compute capability is cached elsewhere or accessed later, there's an inconsistency: the device object still reports a 2-tuple, but compilation uses a 3-tuple. Verify that cached cubins are properly keyed by the 3-tuple to avoid cache misses or incorrect binary selection.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cc argument was removed here because it was the same as the default that got used in _ensure_cc anyway. So this removal saved making the same change in two different places.

Comment on lines 349 to +352

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call was changed from self.get_cubin(cc=device.compute_capability) to self.get_cubin(). While this change ensures the arch suffix is automatically added via _ensure_cc(), it could have subtle implications:

  1. Previously, this explicitly passed the device's CC, which was always a 2-tuple
  2. Now it relies on _ensure_cc(None) to query and augment the CC

This change is likely intentional to ensure arch-specific compilation, but it should be noted that this modifies the cache key used for cubin lookups when cc >= 9.0. Old cached cubins compiled before this PR won't be found (cache invalidation).

module = ctx.create_module_image(
cubin, self._setup_functions, self._teardown_functions
)
Expand Down
6 changes: 5 additions & 1 deletion numba_cuda/numba/cuda/cudadrv/driver.py
Original file line number Diff line number Diff line change
Expand Up @@ -2303,7 +2303,11 @@ def __init__(
lto=None,
additional_flags=None,
):
arch = f"sm_{cc[0]}{cc[1]}"
if len(cc) == 3:
arch = f"sm_{cc[0]}{cc[1]}{cc[2]}"
else:
arch = f"sm_{cc[0]}{cc[1]}"
Comment on lines +2306 to +2309

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the nvrtc.py issue, there's no validation that cc is a properly formed tuple. Consider adding a check:

if not isinstance(cc, tuple) or len(cc) not in (2, 3):
    raise ValueError(f"cc must be a 2-tuple or 3-tuple, got {cc}")

This will provide clearer error messages if invalid input is passed.


self.max_registers = max_registers if max_registers else None
self.lineinfo = lineinfo
self.cc = cc
Expand Down
85 changes: 54 additions & 31 deletions numba_cuda/numba/cuda/cudadrv/nvrtc.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,44 @@ def _get_nvrtc_version():
return (major, minor)


def _verify_cc_tuple(cc):
version = _get_nvrtc_version()
ver_str = lambda version: ".".join(str(v) for v in version)

if len(cc) == 3:
cc, arch = (cc[0], cc[1]), cc[2]
else:
arch = ""
Comment on lines +37 to +40

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable shadowing issue: Line 38 reassigns cc from a 3-tuple to a 2-tuple (cc[0], cc[1]), extracting the arch suffix separately. This shadows the input parameter cc and makes the code harder to follow.

Additionally, this creates a potential bug on line 50 where cc (now the 2-tuple) is used in the filter comparison, but the original 3-tuple meaning is lost. While this works correctly because we're only comparing major/minor versions, the code would be clearer with distinct variable names.

Suggestion: Use distinct variable names:

if len(cc) == 3:
    cc_base = (cc[0], cc[1])
    arch = cc[2]
else:
    cc_base = cc
    arch = ""

# Then use cc_base throughout for comparisons
found = max(filter(lambda v: v <= cc_base, [v for v in supported_ccs]))


if arch not in ("", "a", "f"):
raise ValueError(
f"Invalid architecture suffix '{arch}' in compute capability "
f"{ver_str(cc)}{arch}. Expected '', 'a', or 'f'."
)
Comment on lines +33 to +46

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no validation that cc is a tuple with length 2 or 3. If an invalid input like cc=(9,) (1-tuple) or cc=(9, 0, "a", "extra") (4-tuple) is passed, the function will fail with unclear error messages.

Consider adding validation at the start of the function:

if not isinstance(cc, tuple) or len(cc) not in (2, 3):
    raise ValueError(
        f"Compute capability must be a 2-tuple (major, minor) or "
        f"3-tuple (major, minor, arch), got {cc}"
    )


supported_ccs = get_supported_ccs()
try:
found = max(filter(lambda v: v <= cc, [v for v in supported_ccs]))
except ValueError:
raise RuntimeError(
f"Device compute capability {ver_str(cc)} is less than the "
f"minimum supported by NVRTC {ver_str(version)}. Supported "
"compute capabilities are "
f"{', '.join([ver_str(v) for v in supported_ccs])}."
)

if found != cc:
found = (found[0], found[1], arch)
warnings.warn(
f"Device compute capability {ver_str(cc)} is not supported by "
f"NVRTC {ver_str(version)}. Using {ver_str(found)} instead."
Comment on lines +59 to +63

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misleading warning message. When found != cc (line 59), it means NVRTC doesn't support the exact CC version, so it falls back to a lower supported version. However, the warning message on lines 62-63 says "Device compute capability ... is not supported" which is confusing because:

  1. It's actually the NVRTC toolkit that doesn't support it, not the device
  2. The warning talks about "Using ... instead" but the arch suffix is preserved from the input, which might not make sense for a downgraded CC

For example, if a user requests CC 9.5 with "a" suffix on a toolkit that only supports up to 9.0, the code will return (9, 0, "a"). But the warning message is misleading about what's happening.

Suggestion: Make the warning more accurate:

warnings.warn(
    f"Compute capability {ver_str(cc)}{arch} is not supported by "
    f"NVRTC {ver_str(version)}. Downgrading to {ver_str(found)} "
    f"(closest supported version) with arch suffix '{arch}'."
)

)
else:
found = (cc[0], cc[1], arch)

return found


def compile(src, name, cc, ltoir=False, lineinfo=False, debug=False):
"""
Compile a CUDA C/C++ source to PTX or LTOIR for a given compute capability.
Expand All @@ -38,7 +76,8 @@ def compile(src, name, cc, ltoir=False, lineinfo=False, debug=False):
:type src: str
:param name: The filename of the source (for information only)
:type name: str
:param cc: A tuple ``(major, minor)`` of the compute capability
:param cc: A tuple ``(major, minor)`` or ``(major, minor, arch)`` of the
compute capability
:type cc: tuple
:param ltoir: Compile into LTOIR if True, otherwise into PTX
:type ltoir: bool
Expand All @@ -49,34 +88,18 @@ def compile(src, name, cc, ltoir=False, lineinfo=False, debug=False):
:return: The compiled PTX or LTOIR and compilation log
:rtype: tuple
"""
found = _verify_cc_tuple(cc)
version = _get_nvrtc_version()

ver_str = lambda version: ".".join(str(v) for v in version)
supported_ccs = get_supported_ccs()
try:
found = max(filter(lambda v: v <= cc, [v for v in supported_ccs]))
except ValueError:
raise RuntimeError(
f"Device compute capability {ver_str(cc)} is less than the "
f"minimum supported by NVRTC {ver_str(version)}. Supported "
"compute capabilities are "
f"{', '.join([ver_str(v) for v in supported_ccs])}."
)

if found != cc:
warnings.warn(
f"Device compute capability {ver_str(cc)} is not supported by "
f"NVRTC {ver_str(version)}. Using {ver_str(found)} instead."
)

# Compilation options:
# - Compile for the current device's compute capability.
# - The CUDA include path is added.
# - Relocatable Device Code (rdc) is needed to prevent device functions
# being optimized away.
major, minor = found
major, minor = found[0], found[1]
cc_arch = found[2] if len(found) == 3 else ""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check if len(found) == 3 is redundant because _verify_cc_tuple always returns a 3-tuple (see lines 60 and 66 in the same file). Consider simplifying to:

Suggested change
cc_arch = found[2] if len(found) == 3 else ""
cc_arch = found[2]

This makes the code clearer and avoids the unnecessary conditional.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +99 to +100

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary defensive check: _verify_cc_tuple() always returns a 3-tuple (see lines 60 and 66), so len(found) == 3 will always be True. This check is harmless but redundant.

If you want to keep it for safety, consider simplifying to just found[2] without the conditional, or add a comment explaining why the defensive check exists.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


arch = f"sm_{major}{minor}"
arch = f"sm_{major}{minor}{cc_arch}"

cuda_include_dir = get_cuda_paths()["include_dir"].info
cuda_includes = [f"{cuda_include_dir}"]
Expand Down Expand Up @@ -156,7 +179,7 @@ def write(self, msg):
return result, log


def find_closest_arch(mycc):
def find_closest_arch(cc):
"""
Given a compute capability, return the closest compute capability supported
by the CUDA toolkit.
Expand All @@ -166,17 +189,17 @@ def find_closest_arch(mycc):
"""
supported_ccs = get_supported_ccs()

for i, cc in enumerate(supported_ccs):
if cc == mycc:
for i, supported_cc in enumerate(supported_ccs):
if supported_cc == cc:
# Matches
return cc
elif cc > mycc:
return supported_cc
elif supported_cc > cc:
# Exceeded
if i == 0:
# CC lower than supported
msg = (
"GPU compute capability %d.%d is not supported"
"(requires >=%d.%d)" % (mycc + cc)
"(requires >=%d.%d)" % (cc + supported_cc)
)
Comment on lines 200 to 203

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tuple concatenation syntax error will cause runtime crash. The expression cc + supported_cc attempts to concatenate two tuples for use in string formatting with %, but the format string expects 4 separate values, not a 4-tuple.

This should be:

Suggested change
msg = (
"GPU compute capability %d.%d is not supported"
"(requires >=%d.%d)" % (mycc + cc)
"(requires >=%d.%d)" % (cc + supported_cc)
)
msg = (
"GPU compute capability %d.%d is not supported"
"(requires >=%d.%d)" % (cc[0], cc[1], supported_cc[0], supported_cc[1])
)

Note: This bug exists in both the original and modified code at this location. The refactoring changed the parameter name from mycc to cc but didn't fix the pre-existing bug.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is correct, but fixing it is low-priority and should be done outside this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon re-reading the comment, I think it is bogus.

raise CCSupportError(msg)
else:
Expand All @@ -187,13 +210,13 @@ def find_closest_arch(mycc):
return supported_ccs[-1] # Choose the highest


def get_arch_option(major, minor):
def get_arch_option(major, minor, arch=""):
"""Matches with the closest architecture option"""
if config.FORCE_CUDA_CC:
arch = config.FORCE_CUDA_CC
major, minor = config.FORCE_CUDA_CC
else:
arch = find_closest_arch((major, minor))
return "compute_%d%d" % arch
major, minor = find_closest_arch((major, minor))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When config.FORCE_CUDA_CC is set, the arch parameter is discarded. If a user explicitly passes a 3-tuple with an arch suffix (e.g., (9, 0, "a")), the arch suffix will be lost when FORCE_CUDA_CC overrides the major/minor values.

Consider preserving the arch suffix:

Suggested change
if config.FORCE_CUDA_CC:
arch = config.FORCE_CUDA_CC
major, minor = config.FORCE_CUDA_CC
else:
arch = find_closest_arch((major, minor))
return "compute_%d%d" % arch
major, minor = find_closest_arch((major, minor))
if config.FORCE_CUDA_CC:
major, minor = config.FORCE_CUDA_CC
# arch parameter is preserved from the function argument
else:

This ensures that if someone calls get_arch_option(9, 0, "a") with FORCE_CUDA_CC=(9, 0), they still get "compute_90a" instead of "compute_90". Should the arch suffix be preserved when FORCE_CUDA_CC is set, or is it intentional to reset it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think arch should be discarded when FORCE_CUDA_CC is set but it isn't here. This should be fixed, but not in the way suggested by greptile.

return f"compute_{major}{minor}{arch}"
Comment on lines +213 to +235

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic inconsistency: When config.FORCE_CUDA_CC is set (lines 216-221), the function extracts the arch suffix from fcc and assigns it to the arch parameter. However, this overwrites the arch parameter that was passed into the function, which could be unexpected behavior.

Example problematic scenario:

# User calls: get_arch_option(9, 0, "f")  
# With FORCE_CUDA_CC = (9, 0, "a")
# Result: returns "compute_90a" (ignoring the "f" that was passed in)

The function signature suggests arch="" is a parameter the caller can control, but it gets overwritten when FORCE_CUDA_CC is set.

Recommendation: Either:

  1. Document that arch parameter is ignored when FORCE_CUDA_CC is set, OR
  2. Respect the passed-in arch parameter and only use the major/minor from FORCE_CUDA_CC

Based on the test at line 443-446 which expects the FORCE_CUDA_CC arch to be used, option 1 seems intended, so add documentation.



def get_lowest_supported_cc():
Expand Down
4 changes: 4 additions & 0 deletions numba_cuda/numba/cuda/testing.py
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,10 @@ def skip_unless_cc_75(fn):
return unittest.skipUnless(cc_X_or_above(7, 5), "requires cc >= 7.5")(fn)


def skip_unless_cc_90(fn):
return unittest.skipUnless(cc_X_or_above(9, 0), "requires cc >= 9.0")(fn)


def xfail_unless_cudasim(fn):
if config.ENABLE_CUDASIM:
return fn
Expand Down
47 changes: 46 additions & 1 deletion numba_cuda/numba/cuda/tests/cudadrv/test_cuda_driver.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
# SPDX-License-Identifier: BSD-2-Clause

from ctypes import c_int, sizeof
import cffi
import numpy as np

from numba.cuda.cudadrv.driver import host_to_device, device_to_host, driver
from numba.cuda._compat import (
Expand All @@ -13,10 +15,12 @@

from numba import cuda
from numba.cuda.cudadrv import devices
from numba.cuda.testing import unittest, CUDATestCase
from numba.cuda.testing import unittest, CUDATestCase, skip_unless_cc_90
from numba.cuda.testing import skip_on_cudasim
from numba.core import types
import contextlib


ptx1 = """
.version 1.4
.target sm_10, map_f64_to_f32
Expand Down Expand Up @@ -390,5 +394,46 @@ def test_device_get_uuid(self):
self.assertRegex(dev.uuid, uuid_format)


@skip_unless_cc_90
@skip_on_cudasim("CUDA asm unsupported in the simulator")
class TestAcceleratedArchitecture(CUDATestCase):
def test_device_arch_specific(self):
set_desc = cuda.CUSource("""
#include <cuda_fp16.h>

extern "C" __device__
int set_descriptor(int *out, int* smem) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The C function signature has 2 parameters (int *out, int* smem) but the declaration on line 415 specifies only 1 parameter (types.CPointer(types.int32)). This is a critical mismatch that will cause undefined behavior.

The first parameter int *out is never used in the C code and should be removed. The correct C signature should be:

Suggested change
int set_descriptor(int *out, int* smem) {
int set_descriptor(int* smem) {

This makes the C signature match the Python declaration which expects a single pointer parameter.

unsigned usmem = __cvta_generic_to_shared(smem);
asm volatile("tensormap.replace.tile.rank.shared::cta.b1024.b32 [%0], 2;" :: "r"(usmem));
return 0;
}
""")

set_descriptor = cuda.declare_device(
"set_descriptor",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

types.int32(types.CPointer(types.int32)) signature doesn't match the C function which takes two parameters (int *out, int* smem), but declare_device is only passed one parameter type

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not an error, the Numba calling convention specifies that the return value is stored into the value pointed to by the first argument.

types.int32(types.CPointer(types.int32)),
link=[set_desc],
Comment on lines +413 to +416

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The set_descriptor function has an unused out parameter and doesn't actually set any descriptor—it only executes a tensormap.replace instruction. The function signature and implementation don't match the apparent intent.

If this is intentional for testing that the architecture-specific instruction compiles and runs, consider renaming the function to reflect what it actually does, or remove the unused parameter.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's intentional for testing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this is updating a TMA descriptor, so the name seems appropriate.

Comment on lines +413 to +416

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P0] Function signature mismatch causes runtime error. The C function set_descriptor takes 2 parameters (int *out, int* smem), but the declared signature only specifies 1 parameter. Change to types.int32(types.CPointer(types.int32), types.CPointer(types.int32)).

Comment on lines +405 to +416

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The C function set_descriptor declares two parameters (int *out, int* smem) but the Numba declaration only specifies one parameter types.CPointer(types.int32), and only one argument is passed at line 425. This is a parameter count mismatch.

Looking at the C implementation, the out parameter is never used - only smem is used. The function signature should be corrected to match the actual usage:

extern "C" __device__
int set_descriptor(int* smem) {
    // ... rest of implementation
}

Or if out is intended to be used, the Numba declaration and call site need to be updated to pass both parameters.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because of the Numba ABI.

)
Comment on lines +413 to +417

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function signature is incorrect. The set_descriptor function is declared to take 2 parameters (int *out, int* smem), but the C code at line 406 only declares ONE parameter (int* smem). The out parameter is never used in the C implementation.

The signature should be:

Suggested change
set_descriptor = cuda.declare_device(
"set_descriptor",
types.int32(types.CPointer(types.int32)),
link=[set_desc],
)
set_descriptor = cuda.declare_device(
"set_descriptor",
types.int32(types.CPointer(types.int32)),
link=[set_desc],
)

Or if two parameters are needed, the C code should be updated to actually use both parameters.


ffi = cffi.FFI()

@cuda.jit
def kernel(a):
sm = cuda.shared.array(1, dtype=np.int32)
data_ptr = ffi.from_buffer(sm)
set_descriptor(data_ptr)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P0] Function call missing required parameter. set_descriptor expects 2 arguments but only receives 1. Should be set_descriptor(data_ptr, data_ptr) or similar based on the intended semantics.


# just to prevent optimization:
sm[0] = 2
cuda.syncthreads()
a[0] = sm[0]

a = np.ones(1, dtype=np.int32)

kernel[1, 1](a)

assert a[0] == 2


if __name__ == "__main__":
unittest.main()
30 changes: 15 additions & 15 deletions pixi.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading