Skip to content

Fix CCCL/Jitify compatibility issues for NVRTC compilation#19951

Open
a-hirota wants to merge 1 commit intorapidsai:branch-25.10from
a-hirota:fix-cccl-nvrtc-stdint
Open

Fix CCCL/Jitify compatibility issues for NVRTC compilation#19951
a-hirota wants to merge 1 commit intorapidsai:branch-25.10from
a-hirota:fix-cccl-nvrtc-stdint

Conversation

@a-hirota
Copy link
Contributor

Description

This PR fixes build failures caused by type redeclaration conflicts between CCCL 3.0.2 and
Jitify when compiling with NVRTC.

Problem

When building cuDF with ./build.sh, the JIT preprocessing step fails with errors like:
cuda/std/cstdint(43): error: invalid redeclaration of type name "int_fast16_t"
cuda/std/cstdint(60): error: invalid redeclaration of type name "intptr_t"
limits(149): error: invalid narrowing conversion from "int" to "char"

Root Cause

  1. CCCL's NVRTC-specific stdint type definitions conflict with system stdint.h when both are
    included
  2. Jitify's numeric_limits template fails to compile due to narrowing conversion when CHAR_MAX
    is an int value

Solution

This PR adds two patches that are automatically applied during the CMake build process:

1. CCCL Patch (cccl_jitify_compatibility.patch)

  • Guards type definitions with #ifndef _STDINT_H to prevent redeclaration when stdint.h is
    already included
  • Adds explicit casts to CHAR_MIN/MAX macros to avoid narrowing conversions

2. Jitify Patch (jitify_char_limits.patch)

  • Adds explicit casts in numeric_limits template instantiation

Testing

  • Successfully builds with ./build.sh on Ubuntu with CUDA 13.0
  • JIT preprocessing (make jitify_preprocess_run) completes without errors
  • Patches are applied automatically during the build process

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

This commit addresses build failures caused by type redeclaration
conflicts between CCCL 3.0.2 and Jitify when compiling with NVRTC.

Issues fixed:
1. stdint type redeclarations: CCCL's cstdint header conflicted with
   system stdint.h types (int_fast16_t, intptr_t, etc.)
   - Added _STDINT_H guards to prevent duplicate definitions

2. char limits narrowing conversion: numeric_limits<char> template
   instantiation failed with narrowing conversion error
   - Added explicit casts to CHAR_MIN and CHAR_MAX

Implementation:
- Added CCCL patch to guard type definitions and fix char limits
- Added Jitify patch to fix numeric_limits<char> instantiation
- Integrated patches into CMake build system via rapids-cmake
- Patches are applied automatically during build process

Testing:
- Successfully builds with ./build.sh on Ubuntu with CUDA 13.0
- JIT preprocessing completes without errors
@copy-pr-bot
Copy link

copy-pr-bot bot commented Sep 14, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@a-hirota a-hirota marked this pull request as ready for review September 14, 2025 11:58
@a-hirota a-hirota requested a review from a team as a code owner September 14, 2025 11:58
Copilot AI review requested due to automatic review settings September 14, 2025 11:58
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes NVRTC compilation failures by resolving type redeclaration conflicts between CCCL 3.0.2 and Jitify through automated patch application during the CMake build process.

  • Adds guards to prevent stdint type redeclarations in CCCL when system stdint.h is included
  • Fixes narrowing conversion errors in numeric_limits template instantiations
  • Implements automatic patch application through CMake configuration

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
cpp/cmake/thirdparty/patches/jitify_override.json Configuration to apply Jitify patch for char limits fix
cpp/cmake/thirdparty/patches/jitify_char_limits.patch Patch file adding explicit casts to fix narrowing conversions
cpp/cmake/thirdparty/patches/cccl_override.json Configuration to apply CCCL compatibility patch
cpp/cmake/thirdparty/patches/cccl_jitify_compatibility.patch Patch file adding include guards and fixing char limits
cpp/cmake/thirdparty/get_jitify.cmake Updated to apply Jitify patch during build
cpp/cmake/thirdparty/get_cccl.cmake Updated to apply CCCL patch during build

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

GIT_TAG 44e978b21fc8bdb6b2d7d8d179523c8350db72e5 # jitify2 branch as of 23rd Aug 2025
GIT_SHALLOW FALSE
DOWNLOAD_ONLY TRUE
PATCH_COMMAND git apply --reject --whitespace=fix ${CMAKE_CURRENT_LIST_DIR}/patches/jitify_char_limits.patch || true
Copy link

Copilot AI Sep 14, 2025

Choose a reason for hiding this comment

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

The || true clause will suppress all git apply failures, including legitimate patch application errors. This could mask real issues when the patch fails to apply correctly, leading to silent build failures later.

Suggested change
PATCH_COMMAND git apply --reject --whitespace=fix ${CMAKE_CURRENT_LIST_DIR}/patches/jitify_char_limits.patch || true
PATCH_COMMAND git apply --reject --whitespace=fix ${CMAKE_CURRENT_LIST_DIR}/patches/jitify_char_limits.patch

Copilot uses AI. Check for mistakes.
Comment on lines +19 to +20
+// Only define types if stdint.h hasn't been included
+#ifndef _STDINT_H
Copy link

Copilot AI Sep 14, 2025

Choose a reason for hiding this comment

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

The macro _STDINT_H is implementation-specific and may not be consistent across all compilers and standard libraries. Consider using a more portable approach like checking for specific types or using a project-specific guard macro.

Copilot uses AI. Check for mistakes.
Comment on lines +31 to +32
+// Only define macros if stdint.h hasn't been included
+#ifndef _STDINT_H
Copy link

Copilot AI Sep 14, 2025

Choose a reason for hiding this comment

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

This duplicates the same implementation-specific _STDINT_H guard used for type definitions. The inconsistency in approach (guarding types and macros separately with the same condition) could lead to maintenance issues.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Response to GitHub Copilot Review Comments

GitHub Copilot raised two valid concerns about this implementation. Here's my analysis and rationale for the current approach:

1. Regarding || true in Jitify patch command

Copilot's concern: The || true suppresses all git apply failures, potentially masking legitimate patch application errors.

Current rationale: The || true is necessary because:

  • The patch may be applied multiple times during incremental builds or when switching between clean/dirty build states
  • Without it, builds fail when the patch is already applied
  • The CCCL patch uses rapids-cmake's override system which handles idempotency properly
  • For Jitify, we use CMake's PATCH_COMMAND which doesn't have the same idempotency infrastructure

Alternative considered: Adding a check like git apply --check before applying, but this adds complexity for a temporary fix.

2. Regarding _STDINT_H macro portability

Copilot's concern: The _STDINT_H macro is implementation-specific and may not be consistent across all compilers.

Current rationale:

  • _STDINT_H is used by glibc (Linux), which is cuDF's primary target platform
  • This code only executes in NVRTC context with controlled environment
  • The fix is temporary until upstream CCCL/Jitify resolve the compatibility issue

Future improvement: For broader portability, we could check multiple guards:

#if !defined(_STDINT_H) && !defined(__STDINT_H) && !defined(_STDINT)

Summary

Both concerns represent valid engineering trade-offs. The current implementation prioritizes:
1. Build reliability over perfect error reporting (via || true)
2. Target platform support over universal portability (via _STDINT_H)

These are temporary patches until upstream fixes are available. The build successfully completes with these changes on Ubuntu/CUDA 13.0.

Happy to discuss alternative approaches or make improvements based on maintainer preferences.

@a-hirota a-hirota force-pushed the fix-cccl-nvrtc-stdint branch from 1af0385 to e1cda95 Compare September 14, 2025 13:15
@a-hirota
Copy link
Contributor Author

Update: Force-pushed to revert to working version (commit e1cda95ac7) due to build failures when addressing Copilot feedback.

@davidwendt
Copy link
Contributor

CCCL does not generally want us to add patching like this.
Could you open an issue or PR in the CCCL repo and/or Jitify to fix this there instead?

@a-hirota
Copy link
Contributor Author

Thank you for your feedback. This PR applies temporary patches on the cuDF side to work around a build failure caused by type redeclaration conflicts between CCCL 3.0.2 and Jitify when compiling with NVRTC. I understand that CCCL generally prefers not to apply such patches downstream.

I will open an issue and/or pull request in the NVIDIA/cccl repository and in Jitify to address the underlying problem upstream. In the meantime, could you advise on the best approach to keep cuDF building until the upstream changes are merged? Once I create the issues/PRs, I will share the links here for your review.

Thank you again for your guidance.

@vyasr
Copy link
Contributor

vyasr commented Sep 16, 2025

@lamarrr might be the best person to review this right now since he's done the jitify updates recently including similar compatibility issues like NVIDIA/jitify#147.

@lamarrr
Copy link
Contributor

lamarrr commented Sep 17, 2025

Thanks for the detailed report and patch @a-hirota. As @davidwendt mentioned, we generally avoid making one-off patches. Can you please submit a PR to both JITIFY and CCCL? If not, we can help make the PRs.
Are these actually errors or warnings being raised as errors due to a Werror flag?

@a-hirota
Copy link
Contributor Author

Thanks @lamarrr! These are definitely actual compilation errors, not warnings. The issue is quite extensive - here's a sample of the errors:

cuda/std/cstdint(43): error: invalid redeclaration of type name "int_fast16_t"
cuda/std/cstdint(47): error: invalid redeclaration of type name "uint_fast16_t"
cuda/std/cstdint(60): error: invalid redeclaration of type name "intptr_t"
limits(149): error: invalid narrowing conversion from "int" to "char"
cuda/std/__cmath/isnan.h(48): error: expected an identifier

The problem affects multiple kernel files and causes cascading errors throughout CCCL's math functions (isnan, isinf, etc.) and NVIDIA-specific floating-point types. The compilation hits the 100-error limit, so there are likely even more issues.
If you could help create the PRs for both Jitify and CCCL, that would be incredibly helpful and much faster than me trying to tackle this complex issue. The scope is quite broad and would benefit from your expertise!
Thank you so much for offering to help!

Comment on lines +15 to +65
@@ -30,6 +30,9 @@ _CCCL_PUSH_MACROS
#else // ^^^ !_CCCL_COMPILER(NVRTC) ^^^ / vvv _CCCL_COMPILER(NVRTC) vvv
# include <cuda/std/climits>

+// Only define types if stdint.h hasn't been included
+#ifndef _STDINT_H
+
using int8_t = signed char;
using int16_t = signed short;
using int32_t = signed int;
@@ -63,6 +66,11 @@ using uintptr_t = uint64_t;
using intmax_t = int64_t;
using uintmax_t = uint64_t;

+#endif // _STDINT_H
+
+// Only define macros if stdint.h hasn't been included
+#ifndef _STDINT_H
+
# define INT8_MIN SCHAR_MIN
# define INT16_MIN SHRT_MIN
# define INT32_MIN INT_MIN
@@ -126,6 +134,8 @@ using uintmax_t = uint64_t;

# define INTMAX_C(X) ((::intmax_t)(X))
# define UINTMAX_C(X) ((::uintmax_t)(X))
+
+#endif // _STDINT_H
#endif // ^^^ _CCCL_COMPILER(NVRTC)

_LIBCUDACXX_BEGIN_NAMESPACE_STD
diff --git a/libcudacxx/include/cuda/std/climits b/libcudacxx/include/cuda/std/climits
index a605f2dc5..085205322 100644
--- a/libcudacxx/include/cuda/std/climits
+++ b/libcudacxx/include/cuda/std/climits
@@ -34,10 +34,10 @@ _CCCL_PUSH_MACROS
# define __CHAR_UNSIGNED__ ('\xff' > 0) // CURSED
# if __CHAR_UNSIGNED__
# define CHAR_MIN 0
-# define CHAR_MAX UCHAR_MAX
+# define CHAR_MAX ((char)UCHAR_MAX)
# else
-# define CHAR_MIN SCHAR_MIN
-# define CHAR_MAX SCHAR_MAX
+# define CHAR_MIN ((char)SCHAR_MIN)
+# define CHAR_MAX ((char)SCHAR_MAX)
# endif
# define SHRT_MIN (-SHRT_MAX - 1)
# define SHRT_MAX 0x7fff
--
2.34.1 No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think CCCL is the right place to have these changes.
The casts on the macros are not necessary, the limits are declared in other standard library implementations similarly. I think the cast in the numeric_limits header would be sufficient.
Regarding duplicate declarations, it would be best to handle them within JITIFY, rather than within the actual system standard library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @lamarrr,

Thank you for reviewing my PR and for opening NVIDIA/jitify#150 to address the numeric_limits<char> narrowing issue. As you pointed out, adding one‑off patches in cuDF—especially in CCCL—may not be ideal:contentReference[oaicite:4]{index=4}. My current PR includes a temporary patch for CCCL and a patch for Jitify to keep cuDF building until upstream fixes are available.

Since there is now an upstream fix for Jitify and CCCL does not want downstream patches:contentReference[oaicite:5]{index=5}, I'm thinking about closing this PR rather than keeping a temporary CCCL patch in cuDF. Do you think it makes sense to close it and wait for the Jitify fix to land, or should we keep this open (perhaps removing the CCCL part) until the upstream changes are merged and cuDF is updated?

I’d appreciate your guidance on the best course of action.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants