Repository navigation
Conversation
With GPUCA_SPECIALIZE_THRUST_SORTS (CUDA and HIP), the host-side launch of GPUTPCGMO2Output::sort takes the number of elements from the host copy of the merger's memory, processors()->tpcMerger.NOutputTracksTPCO2(). That count is computed by GPUTPCGMO2Output::prepare on the device, and TransferMemoryResourceLinkToHost brings it to the host asynchronously, on the same stream, right before the sort is launched. The event of the transfer was only waited for after the sort, so the sort very likely got the value from before prepare ran: 0, since RunTPCTrackingMerger clears the merger's memory on the host at its start, and the transfer is queued behind prepare. Then the O2 output tracks keep the order in which prepare wrote them. This goes back to b0155c2, which moved the wait after the sort. With debugLevel >= 1 or serializeGPU the transfer is synchronous, which would hide it. The other sorts that read host counts, SortTracks and SortTracksQPt, wait for their transfer with CondWaitEvent first; do the same here, by waiting before the sort. The wait was already needed before the allocation of the O2 output that follows, so it costs no extra synchronisation. Found by reading the code, while porting these sorts to the Metal backend, which reads the count after draining the stream. Not tested on CUDA or HIP, there is no NVIDIA or AMD GPU here; the CPU build compiles. To check on a GPU: with debugLevel 0, outputTracksTPCO2 should come out ordered by the sort's comparator with this change, and not without it.
Member
Author
|
@davidrohr the claim from my friends is that this actually fixes a genuine bug. The change was made in 2021, so I am a bit skeptical it went unnoticed. Could you have a look? |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With GPUCA_SPECIALIZE_THRUST_SORTS (CUDA and HIP), the host-side launch
of GPUTPCGMO2Output::sort takes the number of elements from the host
copy of the merger's memory, processors()->tpcMerger.NOutputTracksTPCO2().
That count is computed by GPUTPCGMO2Output::prepare on the device, and
TransferMemoryResourceLinkToHost brings it to the host asynchronously,
on the same stream, right before the sort is launched. The event of the
transfer was only waited for after the sort, so the sort very likely
got the value from before prepare ran: 0, since RunTPCTrackingMerger
clears the merger's memory on the host at its start, and the transfer
is queued behind prepare. Then the O2 output tracks keep the order in
which prepare wrote them. This goes back to b0155c2, which moved
the wait after the sort. With debugLevel >= 1 or serializeGPU the
transfer is synchronous, which would hide it.
The other sorts that read host counts, SortTracks and SortTracksQPt,
wait for their transfer with CondWaitEvent first; do the same here, by
waiting before the sort. The wait was already needed before the
allocation of the O2 output that follows, so it costs no extra
synchronisation.
Found by reading the code, while porting these sorts to the Metal
backend, which reads the count after draining the stream. Not tested on
CUDA or HIP, there is no NVIDIA or AMD GPU here; the CPU build compiles.
To check on a GPU: with debugLevel 0, outputTracksTPCO2 should come out
ordered by the sort's comparator with this change, and not without it.