Skip to content

Change to allow cccl/c/parallel/unique_by_key.h to compile by C compiler - #4259

Merged
oleksandr-pavlyk merged 1 commit into
NVIDIA:mainfrom
oleksandr-pavlyk:fix-invalid-C-in-unique-by-key-header
Mar 26, 2025
Merged

Change to allow cccl/c/parallel/unique_by_key.h to compile by C compiler#4259
oleksandr-pavlyk merged 1 commit into
NVIDIA:mainfrom
oleksandr-pavlyk:fix-invalid-C-in-unique-by-key-header

Conversation

@oleksandr-pavlyk

@oleksandr-pavlyk oleksandr-pavlyk commented Mar 25, 2025

Copy link
Copy Markdown
Contributor
  1. Remove noexcept
  2. Use uint64_t instead of unsigned long long (no required to allow compilation by C compiler, but adopted throughout to one day support compilation on Windows)
  3. Add #include <unique_by_key.h> to test_headers.c test file

Description

closes #4258

Checklist

  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

1. Remove noexcept
2. Use uint64_t instead of unsigned long long (no required to allow
   compilation by C compiler, but adopted throughout to one day
   support compilation on Windows)
3. Add `#include <unique_by_key.h>` to `test_headers.c` test file
@oleksandr-pavlyk
oleksandr-pavlyk requested a review from a team as a code owner March 25, 2025 21:29
@oleksandr-pavlyk
oleksandr-pavlyk requested a review from shwina March 25, 2025 21:29
@github-project-automation github-project-automation Bot moved this to Todo in CCCL Mar 25, 2025
@cccl-authenticator-app cccl-authenticator-app Bot moved this from Todo to In Review in CCCL Mar 25, 2025
@oleksandr-pavlyk

Copy link
Copy Markdown
Contributor Author

While at it, is there a reason by num_items argument of cccl_unique_by_key is placed after cccl_operator_t comparison_op and not before as, for example, is done in cccl_merge_sort?

@NaderAlAwar

Copy link
Copy Markdown
Contributor

While at it, is there a reason by num_items argument of cccl_unique_by_key is placed after cccl_operator_t comparison_op and not before as, for example, is done in cccl_merge_sort?

The reason I did it was to follow the order of the arguments in DispatchUniqueByKey::Dispatch()

CUB_RUNTIME_FUNCTION _CCCL_FORCEINLINE static cudaError_t Dispatch(
void* d_temp_storage,
size_t& temp_storage_bytes,
KeyInputIteratorT d_keys_in,
ValueInputIteratorT d_values_in,
KeyOutputIteratorT d_keys_out,
ValueOutputIteratorT d_values_out,
NumSelectedIteratorT d_num_selected_out,
EqualityOpT equality_op,
OffsetT num_items,
cudaStream_t stream,
KernelSource kernel_source = {},
KernelLauncherFactory launcher_factory = {},
MaxPolicyT max_policy = {})

However looking at the API I can see now that num_items comes before, so I think we should maybe switch it here too. I can open another PR but you can also feel free to do it here.

@NaderAlAwar NaderAlAwar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks @oleksandr-pavlyk

@github-actions

Copy link
Copy Markdown
Contributor
🟩 CI finished in 1h 11m: Pass: 100%/3 | Total: 1h 30m | Avg: 30m 09s | Max: 1h 10m | Hits: 95%/328
  • 🟩 cccl_c_parallel: Pass: 100%/2 | Total: 20m 09s | Avg: 10m 04s | Max: 17m 25s | Hits: 95%/328

    🟩 cpu
      🟩 amd64              Pass: 100%/2   | Total: 20m 09s | Avg: 10m 04s | Max: 17m 25s | Hits:  95%/328   
    🟩 ctk
      🟩 12.8               Pass: 100%/2   | Total: 20m 09s | Avg: 10m 04s | Max: 17m 25s | Hits:  95%/328   
    🟩 cudacxx
      🟩 nvcc12.8           Pass: 100%/2   | Total: 20m 09s | Avg: 10m 04s | Max: 17m 25s | Hits:  95%/328   
    🟩 cudacxx_family
      🟩 nvcc               Pass: 100%/2   | Total: 20m 09s | Avg: 10m 04s | Max: 17m 25s | Hits:  95%/328   
    🟩 cxx
      🟩 GCC13              Pass: 100%/2   | Total: 20m 09s | Avg: 10m 04s | Max: 17m 25s | Hits:  95%/328   
    🟩 cxx_family
      🟩 GCC                Pass: 100%/2   | Total: 20m 09s | Avg: 10m 04s | Max: 17m 25s | Hits:  95%/328   
    🟩 gpu
      🟩 rtx2080            Pass: 100%/2   | Total: 20m 09s | Avg: 10m 04s | Max: 17m 25s | Hits:  95%/328   
    🟩 jobs
      🟩 Build              Pass: 100%/1   | Total:  2m 44s | Avg:  2m 44s | Max:  2m 44s | Hits:  92%/164   
      🟩 Test               Pass: 100%/1   | Total: 17m 25s | Avg: 17m 25s | Max: 17m 25s | Hits:  98%/164   
    
  • 🟩 python: Pass: 100%/1 | Total: 1h 10m | Avg: 1h 10m | Max: 1h 10m

    🟩 cpu
      🟩 amd64              Pass: 100%/1   | Total:  1h 10m | Avg:  1h 10m | Max:  1h 10m
    🟩 ctk
      🟩 12.8               Pass: 100%/1   | Total:  1h 10m | Avg:  1h 10m | Max:  1h 10m
    🟩 cudacxx
      🟩 nvcc12.8           Pass: 100%/1   | Total:  1h 10m | Avg:  1h 10m | Max:  1h 10m
    🟩 cudacxx_family
      🟩 nvcc               Pass: 100%/1   | Total:  1h 10m | Avg:  1h 10m | Max:  1h 10m
    🟩 cxx
      🟩 GCC13              Pass: 100%/1   | Total:  1h 10m | Avg:  1h 10m | Max:  1h 10m
    🟩 cxx_family
      🟩 GCC                Pass: 100%/1   | Total:  1h 10m | Avg:  1h 10m | Max:  1h 10m
    🟩 gpu
      🟩 rtx2080            Pass: 100%/1   | Total:  1h 10m | Avg:  1h 10m | Max:  1h 10m
    🟩 jobs
      🟩 Test               Pass: 100%/1   | Total:  1h 10m | Avg:  1h 10m | Max:  1h 10m
    

👃 Inspect Changes

Modifications in project?

Project
CCCL Infrastructure
libcu++
CUB
Thrust
CUDA Experimental
stdpar
python
+/- CCCL C Parallel Library
Catch2Helper

Modifications in project or dependencies?

Project
CCCL Infrastructure
libcu++
CUB
Thrust
CUDA Experimental
stdpar
+/- python
+/- CCCL C Parallel Library
Catch2Helper

🏃‍ Runner counts (total jobs: 3)

# Runner
2 linux-amd64-gpu-rtx2080-latest-1
1 linux-amd64-cpu16

@oleksandr-pavlyk
oleksandr-pavlyk merged commit 6881339 into NVIDIA:main Mar 26, 2025
@github-project-automation github-project-automation Bot moved this from In Review to Done in CCCL Mar 26, 2025
@oleksandr-pavlyk
oleksandr-pavlyk deleted the fix-invalid-C-in-unique-by-key-header branch March 26, 2025 12:58
davebayer pushed a commit to davebayer/cccl that referenced this pull request Apr 7, 2025
…ler (NVIDIA#4259)

1. Remove noexcept
2. Use uint64_t instead of unsigned long long (no required to allow
   compilation by C compiler, but adopted throughout to one day
   support compilation on Windows)
3. Add `#include <unique_by_key.h>` to `test_headers.c` test file
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 this pull request may close these issues.

[BUG] cuda.parallel: unique_by_key.h header should be compilable by C compiler

2 participants