Skip to content

Implement PCG64 as extension#6292

Merged
fbusato merged 26 commits intoNVIDIA:mainfrom
RAMitchell:pcg
Nov 21, 2025
Merged

Implement PCG64 as extension#6292
fbusato merged 26 commits intoNVIDIA:mainfrom
RAMitchell:pcg

Conversation

@RAMitchell
Copy link
Contributor

@RAMitchell RAMitchell commented Oct 20, 2025

Description

Part of #5679

Depends on #6109

@copy-pr-bot
Copy link
Contributor

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

@cccl-authenticator-app cccl-authenticator-app bot moved this from Todo to In Progress in CCCL Oct 20, 2025
#endif // !_CCCL_COMPILER(NVRTC)

private:
using __uint128_type = unsigned __int128;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just use __uint128_t

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still need to implement a fallback int128.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need this on windows right now? I think it would be easier to guard it by #if _CCCL_HAS_INT128() for now and allow this feature on windows once we have our internal int128 fallback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From slack it sounded like we wanted to have a complete implementation. I don't think its too hard so I think we can do it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please, do it in a separate PR

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah lets do this as a followup

@RAMitchell RAMitchell marked this pull request as ready for review October 28, 2025 11:24
@RAMitchell RAMitchell requested a review from a team as a code owner October 28, 2025 11:24
@RAMitchell RAMitchell requested a review from wmaxey October 28, 2025 11:24
@cccl-authenticator-app cccl-authenticator-app bot moved this from In Progress to In Review in CCCL Oct 28, 2025
@RAMitchell
Copy link
Contributor Author

RAMitchell commented Oct 28, 2025

I am a bit unsure about the naming yet and if we would support variants of this engine.

I'm thinking as follows:

pcg64_engine - PCG XSL RR 128/64 algorithm templated on the multiplier/increment
pcg64 - named instance of the above engine with default multiplier/increment
pcg64_discard - a version of pcg64 that can discard a compile time or runtime number of values with each operator(), allowing higher performing "leapfrogging" PRNG in GPU kernels.

@fbusato maybe you have some thoughts?

@github-project-automation github-project-automation bot moved this from In Review to In Progress in CCCL Nov 4, 2025
@fbusato
Copy link
Contributor

fbusato commented Nov 4, 2025

pcg64_engine - PCG XSL RR 128/64 algorithm templated on the multiplier/increment
pcg64 - named instance of the above engine with default multiplier/increment
pcg64_discard - a version of pcg64 that can discard a compile time or runtime number of values with each operator(), allowing higher performing "leapfrogging" PRNG in GPU kernels.

these names are perfectly fine. I'm thinking if there are better names for pcg64_discard but I'm not sure

@fbusato
Copy link
Contributor

fbusato commented Nov 7, 2025

/ok to test a628a81

@miscco
Copy link
Contributor

miscco commented Nov 10, 2025

/ok to test 78e1f4a

@davebayer
Copy link
Contributor

/ok to test a289174

@github-actions

This comment has been minimized.

@miscco
Copy link
Contributor

miscco commented Nov 10, 2025

/ok to test 5d9cdae

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@RAMitchell
Copy link
Contributor Author

@davebayer @fbusato please re-review thanks :)

@github-actions

This comment has been minimized.

public:
using result_type = ::cuda::std::uint64_t;

private:
Copy link
Contributor

Choose a reason for hiding this comment

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

would be better to keep private members at the end of the class

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually not the general advice is to move them to the front, because they are crucial to understand what is in the class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I normally belong to the "end of class" church, but we can do whatever here :)

Copy link
Contributor

Choose a reason for hiding this comment

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

The point being that usually a class implementation starts with constructors and other SMF, so I need to know what are the actual data members. In that case and others I have to jump around to the back of the potentially long definition to know what I am working with

Copy link
Contributor

Choose a reason for hiding this comment

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

interesting, I always thought about user perspective. Users are interested to the interface, not implementation details. Also, the implementation rarely changes.
Anyway, I'm fine with both approaches.

__uint128_t __x_{};

public:
static constexpr result_type default_seed = 0xcafef00dd15ea5e5ULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
static constexpr result_type default_seed = 0xcafef00dd15ea5e5ULL;
static constexpr result_type default_seed = 0xCAFEF00DD15EA5E5ull;

Copy link
Contributor

Choose a reason for hiding this comment

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

why? Is this a rule?

Copy link
Contributor

Choose a reason for hiding this comment

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

definitely not a rule, but suggested by some c++ secure coding guidelines, e.g. Autosar C++14 Rule A2-13-5 https://www.autosar.org/fileadmin/standards/R18-03_R1.4.0/AP/AUTOSAR_RS_CPP14Guidelines.pdf. ull lowercase to have a clear distinction with uppercase digits

@github-actions
Copy link
Contributor

🥳 CI Workflow Results

🟩 Finished in 2h 25m: Pass: 100%/88 | Total: 14h 21m | Max: 1h 35m | Hits: 99%/212945

See results here.

@RAMitchell
Copy link
Contributor Author

@fbusato waiting on you :)

@github-project-automation github-project-automation bot moved this from In Progress to In Review in CCCL Nov 21, 2025
@fbusato fbusato merged commit 6f1829d into NVIDIA:main Nov 21, 2025
108 checks passed
@github-project-automation github-project-automation bot moved this from In Review to Done in CCCL Nov 21, 2025
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.

4 participants