Repository navigation
fix(cuda_core): use nvJitLink-compatible 0/1 values for boolean linker options - #3128
Conversation
📝 Summary
Priority: ➖ Normal Change: Bug fix Merge Risk: 🔵 Low · up to
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
36ecdafb-5184-41f1-a5f6-bad397b13885
📒 Files selected for processing: 2
cuda_core/cuda/core/_linker.pyxcuda_core/tests/test_linker.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
|
/ok to test c539cc2 |
|
leofang
left a comment
There was a problem hiding this comment.
I think this is a breaking change on the nvJitLink side. We should move the bug to their module and ask them to triage/fix.
Agreed, This does look like a behavior change on the nvJitLink side. The old true/false format stopped being accepted starting with a recent build. Let me reach out to the nvJitLink team to understand why and report back before we decide how to proceed. |
|
Confirmed with the nvJitLink team: only 0/1 has ever actually worked. Other values (true/false/True) can be set without error, but fail once an LTO-enabled link actually runs. Fix stays as emitting 0/1. Added an LTO-enabled runtime test, which closes a gap in previous coverage (option-formatting checks alone don't catch this). Ready for re-review. |
|
/ok to test 746df99 |
There was a problem hiding this comment.
🧹 Nitpick comments: 2
cuda_core/tests/test_linker.py: 1
213-236: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winsuggestion: Assert all four formatted options.
The LTO link checks acceptance only. It does not detect a regression to
true/falseon versions that accept both spellings. Add focused serialization assertions and keep the runtime link as the end-to-end check.Suggested test assertion
- options = LinkerOptions(arch="sm_80", debug=True, ftz=True, max_register_count=32) + options = LinkerOptions( + arch="sm_80", + debug=True, + ftz=True, + prec_div=True, + prec_sqrt=True, + fma=True, + max_register_count=32, + ) ... assert "-ftz=1" in options_str + assert "-prec-div=1" in options_str + assert "-prec-sqrt=1" in options_str + assert "-fma=1" in options_strcuda_core/cuda/core/_linker.pyx: 1
430-440: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winsuggestion: Add direct serialization coverage for boolean split-compile inputs.
boolis anintsubtype, and the repository already usesProgramOptions(split_compile=True). The currentLinkerOptionstests use only integers and do not inspect serialized flags, so revertingint(...)would leaveTrueandFalseregressions undetected.+def test_linker_options_split_compile_serializes_booleans(): + options = LinkerOptions(split_compile=True, split_compile_extended=False) + flags = options.as_bytes("nvjitlink") + assert b"-split-compile=1" in flags + assert b"-split-compile-extended=0" in flags
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
a3a7523f-7d13-4280-8400-4bda3c7dce70
📒 Files selected for processing: 1
cuda_core/tests/test_linker.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
leofang
left a comment
There was a problem hiding this comment.
Thanks, Jinfeng.
Approving based on the compiler team's response (that the old code has never worked).
Thanks Leo for the review and for directing us to check with the nvJitLink team first. |
Summary
Close #3254
nvJitLink's input validation was tightened upstream (CUDA Toolkit 13.6) to strictly require
0/1for boolean-valued options and non-negative integers for-split-compile/-split-compile-extended.cuda_core'sLinkerOptions._prepare_nvjitlink_options()was emitting Python-styletrue/falsestrings and rawf"{bool}"("True"/"False"), which the newer nvJitLink rejects withERROR_UNRECOGNIZED_OPTION.This PR changes the nvJitLink backend option formatting to:
-ftz,-prec-div,-prec-sqrt,-fma: emit1/0instead oftrue/false-split-compile,-split-compile-extended: coerce value withint(...)so a bool is sent as1/0rather than the literal string"True"/"False"The NVRTC backend (
--ftz=true/false) and NVVM backend (-ftz=1/0, already correct) are untouched. This only affects the nvJitLink-specific option formatting.Backward compatibility
Verified directly on nvJitLink 13.2.51, 13.4.92 (both released), and a 13.6.0 nightly build:
-ftz=true/-ftz=1trueFAILS,1OK-split-compile=True/=1TrueFAILS,1OKReleased toolkits accept both the old and new formats, so always emitting
1/0is backward compatible and does not regress any currently supported CUDA 13.x toolkit, while fixing compatibility with newer/nightly toolkits.Testing
test_linker.py+test_program.py(241 tests) andtest_program_cache.py(237 tests) pass with zero failures.