[NVVM] Expose nvvm version detection in cuda.bindings.utils.#1837
[NVVM] Expose nvvm version detection in cuda.bindings.utils.#1837abhilash1910 wants to merge 3 commits intoNVIDIA:mainfrom
Conversation
|
pre-commit.ci autofix |
abhilash1910
changed the title
abhilash1910
changed the title
rwgk
added
enhancement
| """ | ||
|
|
||
|
|
||
| def check_nvvm_options(options: Sequence[bytes]) -> bool: |
There was a problem hiding this comment.
For a public Python API, bytes seems unusual here.
Most pythonic would be Sequence[str], but Sequence[str | bytes] would seem fine, too.
The implementation below actually converts bytes to str:
options_list = [opt.decode("utf-8") if isinstance(opt, bytes) else opt for opt in options]
So requiring bytes in the API is especially surprising. My recommendation is to simply use Sequence[str]; then the options_list line can be removed completely.
I also recommend using check_nvvm_compiler_options as the function name, for clarity, especially in the call sites.
There was a problem hiding this comment.
Yes I will update this, I initially thought this to be Sequence[bytes | str] .
| @@ -0,0 +1,93 @@ | |||
| # SPDX-FileCopyrightText: Copyright (c) 2026-2027 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
|
|
||
| if _inspect_function_pointer("__nvvmCreateProgram") == 0: | ||
| return False | ||
| except Exception: |
There was a problem hiding this comment.
This is far too broad. It's very likely to mask bugs. Also the other except Exception further below. Could you please try this in Cursor:
Could you please narrow down the exception catching as much as possible? Please use
ModuleNotFoundErrorfor thenvvmimport; we also want to checkexc.name == "nvvm". If that import works, we don't want to guard the_inspect_function_pointerimport, or the_inspect_function_pointer()call. If those don't work, that's a bug we want to surface.
| options_list = [opt.decode("utf-8") if isinstance(opt, bytes) else opt for opt in options] | ||
| nvvm.verify_program(program, len(options_list), options_list) | ||
| nvvm.compile_program(program, len(options_list), options_list) | ||
| except Exception: |
There was a problem hiding this comment.
If you're compiling anyway, do you actually need the verify_program() call?
I'm thinking the try should just be around this one line:
try:
nvvm.compile_program(prog, len(options), options)
except nvvm.nvvmError as e:
# can we add something here to ensure we're not masking errors other than invalid options?I believe it's really important to take great care that we're not masking actual errors; e.g. the hard-wired _PRECHECK_NVVM_IR might need tweaks for future GPU generations. If we're simply reporting any error as "invalid compiler option", it'll potentially take someone downstream a long time to drill down all the way back here.
| >>> check_nvvm_options([b"-arch=compute_90", b"-numba-debug"]) | ||
| True # if -numba-debug is supported by the installed libNVVM |
There was a problem hiding this comment.
Could we use a non-numba example here?
| from cuda.bindings.utils import check_nvvm_options | ||
|
|
||
| return check_nvvm_options([f"-arch={arch}".encode()]) | ||
| except Exception: |
There was a problem hiding this comment.
It's only a test, therefore not nearly as critical as in the production code, but we may miss regressions if this isn't handled with similar care.
3 participants
Description
Fixes issue #1457 . Provides an api call to
cuda.bindings.utilsto check the version of nvvm.@leofang @rwgk pinging for review.