Skip to content

Conversation

@viralbhadeshiya
Copy link
Contributor

@viralbhadeshiya viralbhadeshiya commented Oct 10, 2025

Description

closes #6104

  • Removed duplicated math function from Thrust
  • Moved all duplicated math function call from Thrust to cuda/std or cuda

Checklist

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

@copy-pr-bot
Copy link
Contributor

copy-pr-bot bot commented Oct 10, 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.

@github-project-automation github-project-automation bot moved this to Todo in CCCL Oct 10, 2025
@cccl-authenticator-app cccl-authenticator-app bot moved this from Todo to In Review in CCCL Oct 10, 2025
Copy link
Contributor

@bernhardmgruber bernhardmgruber left a comment

Choose a reason for hiding this comment

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

Please remove

#include <thrust/detail/integer_math.h>

from files where we no longer need it.

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.

Thanks a lot for working on this issue. It is already looking great.

I found one incorrect replacement and some potential conversion issues

@github-project-automation github-project-automation bot moved this from In Review to In Progress in CCCL Oct 11, 2025
@viralbhadeshiya
Copy link
Contributor Author

@bernhardmgruber @miscco all changes that you guys suggested is done, lmk if there are more Changes. :)

@miscco
Copy link
Contributor

miscco commented Oct 13, 2025

@viralbhadeshiya This looks great 🎉

Thanks a lot for working on this.

I added a few minor changes:

  • dropped unused is_odd
  • improved is_negative
  • dropped two more includes

@miscco
Copy link
Contributor

miscco commented Oct 13, 2025

/ok to test 07b811c

@github-project-automation github-project-automation bot moved this from In Progress to In Review in CCCL Oct 15, 2025
@miscco
Copy link
Contributor

miscco commented Oct 15, 2025

/ok to test 4261f1f

@github-actions

This comment has been minimized.

@miscco
Copy link
Contributor

miscco commented Oct 16, 2025

/ok to test 34fc385

@github-actions

This comment has been minimized.

@miscco
Copy link
Contributor

miscco commented Oct 17, 2025

@fbusato There is an issue with thrust::pool_options

I suspect there is a differing behavior for is_power_of_two could you please inevestigate why it differs?

@fbusato
Copy link
Contributor

fbusato commented Oct 17, 2025

::cuda::is_power_of_two correctly evaluates 0 as non-power of two, while thrust::detail::is_power_of_2() misses the check x != 0.

This causes a different behavior in !::cuda::is_power_of_two(alignment), which is initialized to 0 by default.
https://github.com/NVIDIA/cccl/pull/6188/files#diff-d85bf89d70ec2f2a069c4c0ffeccd2722fe2a108a012566503392885900f40a1R115

@fbusato fbusato assigned viralbhadeshiya and unassigned fbusato Oct 17, 2025
@fbusato
Copy link
Contributor

fbusato commented Oct 17, 2025

@viralbhadeshiya my suggestion is just to add the condition alignment != 0 && !::cuda::is_power_of_two(alignment)

@viralbhadeshiya
Copy link
Contributor Author

::cuda::is_power_of_two correctly evaluates 0 as non-power of two, while thrust::detail::is_power_of_2() misses the check x != 0.

This causes a different behavior in !::cuda::is_power_of_two(alignment), which is initialized to 0 by default. https://github.com/NVIDIA/cccl/pull/6188/files#diff-d85bf89d70ec2f2a069c4c0ffeccd2722fe2a108a012566503392885900f40a1R115

@fbusato would this similar changes also required in !::cuda::is_power_of_two(smallest_block_size) & !::cuda::is_power_of_two(largest_block_size)?

@fbusato
Copy link
Contributor

fbusato commented Oct 17, 2025

yes, but I'm not sure that this edge case is evaluated in the test case. You can also run the test locally to make sure it works

@viralbhadeshiya
Copy link
Contributor Author

yes, but I'm not sure that this edge case is evaluated in the test case. You can also run the test locally to make sure it works

Okay made changes, unfortunately my remote server which had NVIDIA device is facing some issue so could not run tests. but changes are done.

@fbusato
Copy link
Contributor

fbusato commented Oct 17, 2025

/ok to test 0ab0b72

@github-actions
Copy link
Contributor

🥳 CI Workflow Results

🟩 Finished in 4h 29m: Pass: 100%/118 | Total: 3d 10h | Max: 4h 28m | Hits: 90%/166971

See results here.

@bernhardmgruber bernhardmgruber merged commit 5303f65 into NVIDIA:main Oct 20, 2025
128 checks passed
@github-project-automation github-project-automation bot moved this from In Review to Done in CCCL Oct 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[FEA]: Drop duplicated math functions from Thrust

4 participants