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

[RFC] Consolidate assertions in CCCL #2381

Closed
bernhardmgruber opened this issue Sep 6, 2024 · 1 comment · Fixed by #2382
Closed

[RFC] Consolidate assertions in CCCL #2381

bernhardmgruber opened this issue Sep 6, 2024 · 1 comment · Fixed by #2382
Assignees

Comments

@bernhardmgruber
Copy link
Contributor

As a developer, it is currently unclear to me how to write assertions in host and device code that result in everybody's happiness. Also, the current use of assertions is fragmented between the various subprojects. We have different assertions:

  • assert from <cassert>, which is used in libcu++ unit tests, some CUB unit test, and sparsely in Thrust host and device code. It's also used by libcu++ in the default branches of switch statements which are not reachable.
  • _LIBCUDACXX_DEBUG_ASSERT, which is defined to _LIBCUDACXX_ASSERT if _LIBCUDACXX_ENABLE_DEBUG_MODE. However _LIBCUDACXX_ENABLE_DEBUG_MODE is not enabled anywhere.
  • _LIBCUDACXX_ASSERT, which is only enabled if _LIBCUDACXX_ENABLE_ASSERTIONS is enabled, which is only enabled if _LIBCUDACXX_DEBUG or _LIBCUDACXX_ENABLE_ASSERTIONS is defined. The former may be defined by config.py for test (not clear to me). The latter is pointed out in a comment in one libcu++ test, but otherwise not defined.
  • LIBCPP_ASSERT only used in libcu++ tests

Related, we have different debug levels:

  • CUB has different CUB_DETAIL_DEBUG_LEVELs which include CUB_DETAIL_DEBUG_LEVEL_HOST_ASSERTIONS and CUB_DETAIL_DEBUG_LEVEL_DEVICE_ASSERTIONS, and also CUB_DEBUG_DEVICE_ASSERTIONS and CUB_DEBUG_HOST_ASSERTIONS. All of which seem unused in the code base.
  • libcu++ has _LIBCUDACXX_DEBUG_LEVEL, enabled by _LIBCUDACXX_DEBUG which is used in two places.

Towards users, we should provide at least:

  • Best user experience, by checking and reporting failures as much as possible without severly impacting runtime in debug builds at zero configuration effort (i.e. no defining of extra macros that nobody remembers)
  • Maximum performance in release builds (probably means no assertions)

assert(...) already delivers this today by having assertions enabled by default (good user experience) and allowing to disable those by defining NDEBUG for release builds, which many build systems do by default today (e.g. cmake). However, it seems we also have a fair share of command line users used to getting a fully optimized build out of a plain invocation of nvcc source.cu. This is at odds with assert(...). We may therefore need a new macro that is defined for debug builds, enabling all assertions, and the absence of it disables assertions.

Furthermore, since device assertions are potentially more expensive, since device code in CCCL is generally where we have performance sensitive code, we may want to have dedicated macros for switching on device or host side macros separately.

We may also consider deriving the value of the assert enabling macro from compiler predefined macros like __CUDACC_DEBUG__ (nvcc) or __OPTIMIZE__ (gcc) to detect a debug or release build. But this may be more surprising then helpful.

In any case, those macros and assertions should then be used consistently throughout CCCL, exposed in cmake and the documentation, and enabled (for some builds at least) in the CI.

Related issues: #969 (argues that assertions should be opt-in by default), #959 (raises concerns about plain assert in stdlib code)

@github-project-automation github-project-automation bot moved this to Todo in CCCL Sep 6, 2024
@gevtushenko gevtushenko moved this from Todo to In Progress in CCCL Oct 2, 2024
@cccl-authenticator-app cccl-authenticator-app bot moved this from In Progress to Done in CCCL Oct 23, 2024
@jrhemstad
Copy link
Collaborator

closed by #2382

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants