Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

_LIBCUDACXX_CUDACC_VER major value needs to be computed as the seventh digit #225

Merged

Conversation

robertmaynard
Copy link
Collaborator

The introduction of an extra digit to handle minor CUDACC versions greater than 100 forgot to also increment the major value. Since this meant that all values of _LIBCUDACXX_CUDACC_VER stayed as 6 digits, but we compared against 7 digit values no CUDA versions reported decimal 128 support.

The introduction of an extra digit to handle minor CUDACC versions
greater than 100 forgot to also increment the major value. Since
this meant that all values of _LIBCUDACXX_CUDACC_VER stayed as
6 digits, but we compared against 7 digit values no CUDA versions
reported decimal 128 support.
@robertmaynard robertmaynard changed the title _LIBCUDACXX_CUDACC_VER major value is computed as the seventh digit _LIBCUDACXX_CUDACC_VER major value needs to be computed as the seventh digit Nov 2, 2021
@jrhemstad
Copy link
Collaborator

Yeesh, math is hard. I think this is the 4th time we've had to correct this :)

Makes me feel like we should abandon trying to have a single representative number for the CUDA version. Is there an easier way to write <11.5 by just looking at the major/minor components?

#if defined(_LIBCUDACXX_COMPILER_MSVC) || (defined(_LIBCUDACXX_CUDACC_VER) && ((_LIBCUDACXX_CUDACC_VER_MAJOR < 11) ||  (_LIBCUDACXX_CUDACC_VER_MAJOR == 11 && _LIBCUDACXX_CUDACC_VER_MINOR < 5)) )
#  define _LIBCUDACXX_HAS_NO_INT128
#endif

@robertmaynard
Copy link
Collaborator Author

Yeesh, math is hard. I think this is the 4th time we've had to correct this :)

Makes me feel like we should abandon trying to have a single representative number for the CUDA version. Is there an easier way to write <11.5 by just looking at the major/minor components?

#if defined(_LIBCUDACXX_COMPILER_MSVC) || (defined(_LIBCUDACXX_CUDACC_VER) && ((_LIBCUDACXX_CUDACC_VER_MAJOR < 11) ||  (_LIBCUDACXX_CUDACC_VER_MAJOR == 11 && _LIBCUDACXX_CUDACC_VER_MINOR < 5)) )
#  define _LIBCUDACXX_HAS_NO_INT128
#endif

That is a possibility. I don't know what libcudacxx back-compat policy is, but we might need to keep _LIBCUDACXX_CUDACC_VER as a 6 digit number and use your check

@wmaxey wmaxey self-assigned this Nov 2, 2021
@wmaxey
Copy link
Member

wmaxey commented Nov 2, 2021

Nobody should be using this value downstream. It's certainly not documented. It's purely an internal computation.

Visually I'd prefer the above solution, but I noticed a change I made elsewhere that has a competing idea.

from <cuda/std/chrono>:

#if defined(_LIBCUDACXX_COMPILER_NVCC)
#  if (CUDART_VERSION >= 11050)
#    pragma nv_diag_suppress cuda_demote_unsupported_floating_point
#  else
#    pragma diag_suppress cuda_demote_unsupported_floating_point
#  endif
#endif

@robertmaynard
Copy link
Collaborator Author

Nobody should be using this value downstream. It's certainly not documented. It's purely an internal computation.

Visually I'd prefer the above solution, but I noticed a change I made elsewhere that has a competing idea.

from <cuda/std/chrono>:

#if defined(_LIBCUDACXX_COMPILER_NVCC)
#  if (CUDART_VERSION >= 11050)
#    pragma nv_diag_suppress cuda_demote_unsupported_floating_point
#  else
#    pragma diag_suppress cuda_demote_unsupported_floating_point
#  endif
#endif

The current usage of CUDACC_VER_MAJOR / CUDACC_VER_MINOR captures both clang and nvcc, while moving over to _LIBCUDACXX_COMPILER_NVCC would drop clang. Now that actually might be desired as I don't know if clang + cuda 11.5 is sufficient for decimal 128 support.

@wmaxey
Copy link
Member

wmaxey commented Nov 2, 2021

Right, I would be wary of presuming it for other compilers. I'm happy to stick with the current solution.

@wmaxey wmaxey added the testing: internal ci in progress Currently testing on internal NVIDIA CI (DVS). label Nov 3, 2021
@wmaxey wmaxey self-requested a review November 3, 2021 19:43
@wmaxey wmaxey added testing: internal ci passed Passed internal NVIDIA CI (DVS). and removed testing: internal ci in progress Currently testing on internal NVIDIA CI (DVS). labels Nov 3, 2021
@wmaxey wmaxey merged commit 5834a3b into NVIDIA:main Nov 3, 2021
@robertmaynard robertmaynard deleted the correct_cudacxx_version_computation branch November 3, 2021 20:04
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request Nov 4, 2021
This brings the libcudacxx patch inline with the proposed upstream changes ( NVIDIA/libcudacxx#225 ).

This is needed so that CUDA versions are correctly computed and decimal-128 support is enabled when using CUDA 11.5+

Authors:
  - Robert Maynard (https://github.com/robertmaynard)

Approvers:
  - Conor Hoekstra (https://github.com/codereport)
  - Mark Harris (https://github.com/harrism)

URL: #9579
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
testing: internal ci passed Passed internal NVIDIA CI (DVS).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants