New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor track initialization code #240
Refactor track initialization code #240
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work Amanda! Thanks for slogging through the tedium...
src/sim/TrackInitInterface.hh
Outdated
//// DATA //// | ||
|
||
Items<Primary> primaries; //!< Primary partiicles | ||
size_type storage_factor = 3; //!< Initializer/parent storage per tracks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might need a clang-format here?
src/sim/TrackInitUtils.cc
Outdated
{ | ||
CELER_EXPECT(params); | ||
CELER_EXPECT(*data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure, data && *data
src/sim/TrackInterface.hh
Outdated
= ParamsData<Ownership::const_reference, MemSpace::device>; | ||
using StateDeviceRef = StateData<Ownership::reference, MemSpace::device>; | ||
|
||
#ifndef __CUDA_ARCH__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of #226 we no longer have to bother with the CUDA_ARCH complier exclusion, to simplify things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot about that, thanks!
src/sim/detail/InitializeTracks.cu
Outdated
// most recently added and therefore the ones that still might have a | ||
// parent they can copy the geometry state from. | ||
const TrackInitializer& init | ||
= inits.initializers[ThreadId(inits.initializers.size() - tid.get() - 1)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this uses unsigned integer arithmetic which can overflow, could you check this by inserting:
CELER_ASSERT(tid.get() + 1 <= inits.initializers.size());
and same right before vac_id
?
src/sim/detail/InitializeTracks.cu
Outdated
// Copy the geometry state from the parent for improved | ||
// performance | ||
ThreadId parent_id | ||
= inits.parents[ThreadId(inits.parents.size() - tid.get() - 1)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here -- actually at this point I think you should define a
inline CELER_FUNCTION ThreadId from_back(size_type size, ThreadId cur_thread)
and reuse it in the three places (and the assertion can be a CELER_EXPECT
at the top of the function)
* Use collections for track initializer interface * Separate track initialization data storage from functionality * Template TrackInterface on Ownership and MemSpace and remove State/ParamStore * Address reviewer feedback
This converts the rest of the track initialization data to use
Collection
s and separates the storage of the data from the functionality.