build: unify compiler flags - #2998
Merged
Merged
Conversation
Contributor
github-actions
Bot
added
cuda.bindings
Contributor
Author
|
/ok to test 8e06f2c |
Contributor
|
Andy-Jost
reviewed
Oct 2, 2026
Comment on lines
+176
to
+183
| assert "-std=c++14" in cargs | ||
| assert "-Wno-deprecated-declarations" in cargs | ||
| assert "-g0" in cargs | ||
| assert "-O2" in cargs | ||
| assert "-O3" not in cargs | ||
| assert "-fpermissive" not in cargs | ||
| assert "-fno-var-tracking-assignments" not in cargs | ||
| assert "-Wl,--strip-all" in largs |
Contributor
There was a problem hiding this comment.
I'm not opposed to this kind of test, but it seems unnecessary
Andy-Jost
approved these changes
Oct 2, 2026
Reuse the wheel Cython cache in tests/cython/build_tests.py so the new test stage can hit CUDA_PYTHON_CYTHON_CACHE_DIR. (cherry picked from commit 392178690f26c7d7cb8117a5556a70f2c728c321)
Close the audit from NVIDIA#1882. The two packages' Linux and MSVC flag sets had drifted for legacy reasons; both now go through the same structure. Changes in cuda-bindings: - Drop -fpermissive and -fno-var-tracking-assignments (gcc-only; not needed by current Cython-generated C++; confirmed by a rebuild). - Change -O3 to -O2 (consistent with cuda-core; -O3 was not the source of the measured launch_{256,512}_args latency difference vs c++17). - Add /std:c++14 and /O2 on MSVC (was missing entirely). - Split '-std=c++14 -Wno-deprecated-declarations' onto separate lines with an explanatory comment on each. Changes in cuda-core: - Add /O2 on MSVC (modern setuptools no longer forces /Ox). - Add comments at both the MSVC and Linux flag sites explaining why c++17 is required (structured bindings / if constexpr in _cpp/). Tests: - Update the bindings TestResolveToolchain: replace test_gnu_keeps_gcc_only_flags with test_linux_opt_flag_set (asserts -O2, not -O3; no gnu-only flags); add test_msvc_opt_flag_set; update test_gnu_sets_env_and_flags assertions. - Add test_linux_opt_flag_set and test_msvc_opt_flag_set to the core TestResolveToolchain; update the comment in test_gnu_sets_env_and_flags.
juenglin
force-pushed
the
consolidate-toolchain-helpers
branch
from
October 2, 2026 17:23
8e06f2c to
a04079f
Compare
juenglin
enabled auto-merge (squash)
October 2, 2026 17:28
Contributor
Author
|
/ok to test a04079f |
juenglin
added a commit
to juenglin/cuda-python
that referenced
this pull request
Oct 2, 2026
…in-helpers2 Keep the _build_shared refactor where it overlapped NVIDIA#2998's inlined flag and test copies now on main.
7 of 8 tasks
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
2 participants
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.
Summary
Closes #1882. Implements part of #2965 (flag unification), which this PR supersedes.
The two packages' compiler flag sets had drifted for legacy reasons; this PR audits the differences and aligns them.
Changes
cuda_bindings/build_hooks.py-fpermissiveand-fno-var-tracking-assignments. These were gcc-only and not needed by the current Cython-generated C++; confirmed by a clean rebuild after removal.-O3to-O2, consistent withcuda_core. The-O3→-O2step is not the source of the measuredlaunch_{256,512}_argslatency difference vs.-std=c++17; the C++ standard is./std:c++14and/O2on MSVC (previously missing entirely).-Wno-deprecated-declarationsis kept.cuda_core/build_hooks.py/O2on MSVC. Modern setuptools no longer forces/Ox, so the optimization level must be set explicitly for symmetry with Linux-O2.structured bindingsandif constexprincuda/core/_cpp/).Not addressed in this PR (intentional differences noted in #1882):
-g -O0 -D _GLIBCXX_ASSERTIONS) was already present in both packages before this PR.binding,warn.deprecated.IF) are out of scope.Tests
TestResolveToolchainin both packages updated: bindings'test_gnu_keeps_gcc_only_flagsreplaced bytest_linux_opt_flag_set(asserts-O2, no-O3, no gnu-only flags);test_msvc_opt_flag_setadded to both.Test plan
pytest cuda_bindings/tests/test_build_hooks.py --noconftest— 56 passed, 2 skippedpytest cuda_core/tests/test_build_hooks.py --noconftest— 111 passed, 2 skippedcuda_bindingsrebuilt cleanly with the default gnu toolchain — no new errors or warningsruff check+ruff format --checkclean on all touched files