Skip to content
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

Merged
merged 7 commits into from
Jan 6, 2021

Conversation

StephanTLavavej
Copy link
Member

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 with constexpr. I looked for warning pragmas in the immediate vicinity and didn't see any. (In some cases, the conditions had been extracted into constexpr bools, 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!

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Dec 18, 2020
@StephanTLavavej StephanTLavavej marked this pull request as ready for review December 18, 2020 06:16
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner December 18, 2020 06:16
Copy link
Contributor

@miscco miscco left a 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

Copy link
Contributor

@cbezault cbezault left a 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.

@AlexGuteniev
Copy link
Contributor

Isn't if constexpr C++17 feature that is disabled in C++14 mode?

@miscco
Copy link
Contributor

miscco commented Dec 20, 2020

Isn't if constexpr C++17 feature that is disabled in C++14 mode?

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

Copy link
Contributor

@mnatsuhara mnatsuhara left a comment

Choose a reason for hiding this comment

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

🔥

@fodinabor
Copy link

fodinabor commented Jun 1, 2021

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)
And as the Clang version 11.0.0+ seems to be supported otherwise (implements if constexpr), it'd be great if we could just add a check whether clang is used or nvcc (or nvc++..)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants