-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Always use if constexpr
, require CUDA 10.1 Update 2
#1544
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it strange that this makes me super happy?
Dancing a remove-old-cruft happy dance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I both looked over this manually and used grep to see if anything had been missed so as far as I can tell this looks good.
Isn't |
It is, but one can suppress that warning and it has been decided that it is worth it. The only reason that the workaround was still there is that the old CUDA did not even support it at all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
This breaks CUDA with any Clang version that does not report CUDA > 10.1 (i.e. Clang from 11.0.0 to main can build with CUDA 10.1+ but always reports 10.1) |
Works towards #189.
For ease of reviewing, this is organized into separate commits (which will be squashed).
First,
yvals_core.h
now requires CUDA 10.1 Update 2. Note that VS 2019 was first supported by CUDA 10.1 RTW, so only it and CUDA 10.1 Update 1 are being blocked here. There were no objections when I discussed this planned policy change in the past few months.Then, this replaces
_CONSTEXPR_IF
withconstexpr
. I looked for warning pragmas in the immediate vicinity and didn't see any. (In some cases, the conditions had been extracted intoconstexpr bool
s, both to improve clarity and to avoid warnings; I left those alone.)Next, this drops all of the
_HAS_IF_CONSTEXPR
logic, resulting in 2000+ lines of removed code. In a few cases, this results in clang-format unwrapping of} else {
, but nothing major.This also drops the definition of
_HAS_IF_CONSTEXPR
itself - as usual, we are assuming that users have wisely avoided taking dependencies on undocumented and unsupported STL-internal macros.Finally, this removes
/D_HAS_IF_CONSTEXPR=0
test coverage, which was the primary way we avoided regressing these duplicate codepaths (with the old CUDA 9.2 coverage being a secondary way). These configurations were near-identical duplicates of adjacent configurations, so we weren't relying on them for unique coverage otherwise. (In a few cases, the adjacent configurations had accumulated some options that weren't applied to the/D_HAS_IF_CONSTEXPR=0
duplicates, but this is not a concerning issue - the absence of such options is still being tested by all of the other configurations.)As a result, this will speed up testing by some amount.
This PR does not attempt to replace any occurrences of tag dispatch with
if constexpr
; that can be saved for future cleanups. Also, this will probably cause merge conflicts with in-flight PRs, but they should be simple to resolve, and removing these duplicate codepaths will reduce our maintenance burden. It's time!