GPU: Parallize track merger resolve kernel.

[fweig]

This PR replaces the serial resolve kernel with a parallel version, that runs in 2 steps:

1. Find the connected components of the track slices.
2. Resolve the connected components in parallel.

I got about a ~40 times speedup over the serial version in my tests.

[davidrohr]

Code looks good, will check the output tomorrow.

[davidrohr]

Can't we do 1 push and later on 1 pop?

[fweig]

ah yeah, totally. Didn't pay attention to how push works. Removed the second push.

[davidrohr]

@fweig : This looks pretty good. Results are correct and it is pretty fast. Just one more comment. I would suggest to apply the below patch:

diff --git a/GPU/Common/GPUDefGPUParameters.h b/GPU/Common/GPUDefGPUParameters.h
index 55c5937cace..3f0fa639e85 100644
--- a/GPU/Common/GPUDefGPUParameters.h
+++ b/GPU/Common/GPUDefGPUParameters.h
@@ -96,7 +96,7 @@
   #define GPUCA_LB_GPUTPCGMMergerResolve_step0 256
   #define GPUCA_LB_GPUTPCGMMergerResolve_step1 256
   #define GPUCA_LB_GPUTPCGMMergerResolve_step2 256
-  #define GPUCA_LB_GPUTPCGMMergerResolve_step3 256
+  #define GPUCA_LB_GPUTPCGMMergerResolve_step3 256, 4
   #define GPUCA_LB_GPUTPCGMMergerClearLinks 256
   #define GPUCA_LB_GPUTPCGMMergerMergeWithinPrepare 256
   #define GPUCA_LB_GPUTPCGMMergerMergeSlicesPrepare 256, 2
diff --git a/GPU/GPUTracking/Global/GPUChainTracking.cxx b/GPU/GPUTracking/Global/GPUChainTracking.cxx
index f0a6c17aa18..53914f32728 100644
--- a/GPU/GPUTracking/Global/GPUChainTracking.cxx
+++ b/GPU/GPUTracking/Global/GPUChainTracking.cxx
@@ -1673,11 +1673,10 @@ void GPUChainTracking::RunTPCTrackingMerger_MergeBorderTracks(char withinSlice,
 
 void GPUChainTracking::RunTPCTrackingMerger_Resolve(char useOrigTrackParam, char mergeAll, GPUReconstruction::krnlDeviceType deviceType)
 {
-  runKernel<GPUTPCGMMergerResolve, 0>(GetGridBlk(BlockCount(), 0, deviceType), krnlRunRangeNone, krnlEventNone);
-  runKernel<GPUTPCGMMergerResolve, 1>(GetGridBlk(BlockCount(), 0, deviceType), krnlRunRangeNone, krnlEventNone);
-  runKernel<GPUTPCGMMergerResolve, 2>(GetGridBlk(BlockCount(), 0, deviceType), krnlRunRangeNone, krnlEventNone);
-  // TODO: Determine number of blocks from block size to fully occupy gpu
-  runKernel<GPUTPCGMMergerResolve, 3>(GetGridBlk(BlockCount() * 4, 0, deviceType), krnlRunRangeNone, krnlEventNone, useOrigTrackParam, mergeAll);
+  runKernel<GPUTPCGMMergerResolve, 0>(GetGridAuto(0, deviceType), krnlRunRangeNone, krnlEventNone);
+  runKernel<GPUTPCGMMergerResolve, 1>(GetGridAuto(0, deviceType), krnlRunRangeNone, krnlEventNone);
+  runKernel<GPUTPCGMMergerResolve, 2>(GetGridAuto(0, deviceType), krnlRunRangeNone, krnlEventNone);
+  runKernel<GPUTPCGMMergerResolve, 3>(GetGridAuto(0, deviceType), krnlRunRangeNone, krnlEventNone, useOrigTrackParam, mergeAll);
 }
 
 int GPUChainTracking::RunTPCTrackingMerger(bool synchronizeOutput)

The second parameter in the define defines the minimum number of blocks the GPU must be able to run for this kernel. This will set CUDA launch bounds to impose restriction to the register allocation, allowing the GPU to really run that many blocks.
The GetGridAuto will just derrive the Grid size (and block size) from the launch bounds. Thred count will be thread count from the launch bounds, block cound will be number of multiprocessors on the GPU device * minBlockCount parameter in the launch bounds. This should guarantee full GPU utilization.

[davidrohr]

@fweig : I have seen some random crashes, and running it through valgrind I see some access to uninitialized memory:

==17471== Conditional jump or move depends on uninitialised value(s)
==17471==    at 0x4A3345A: o2::gpu::GPUTPCGMMerger::ResolveFindConnectedComponentsHook(int, int, int, int) (GPUTPCGMMerger.cxx:937)
==17471==    by 0x4A5A9FA: void o2::gpu::GPUTPCGMMergerResolve::Thread<1>(int, int, int, int, o2::gpu::GPUTPCGMMergerResolve::GPUSharedMemory&, o2::gpu::GPUTPCGMMerger&) (GPUTPCGMMergerGPU.cxx:78)
==17471==    by 0x4A7CD1F: int o2::gpu::GPUReconstructionCPUBackend::runKernelBackend<o2::gpu::GPUTPCGMMergerResolve, 1>(o2::gpu::GPUReconstruction::krnlSetup&) (GPUReconstructionCPU.cxx:83)
==17471==    by 0x4A79516: o2::gpu::GPUReconstructionKernels<o2::gpu::GPUReconstructionCPUBackend>::runKernelImpl(o2::gpu::GPUReconstruction::classArgument<o2::gpu::GPUTPCGMMergerResolve, 1>, o2::gpu::GPUReconstruction::krnlSetup&) (GPUReconstructionKernels.h:37)
==17471==    by 0x4AC951B: int o2::gpu::GPUReconstructionCPU::runKernel<o2::gpu::GPUTPCGMMergerResolve, 1, -1>(o2::gpu::GPUReconstruction::krnlExec const&, o2::gpu::GPUReconstruction::krnlRunRange const&, o2::gpu::GPUReconstruction::krnlEvent const&) (GPUReconstructionCPU.h:283)
==17471==    by 0x4AB353D: int o2::gpu::GPUChain::runKernel<o2::gpu::GPUTPCGMMergerResolve, 1, -1>(o2::gpu::GPUReconstruction::krnlExec const&, o2::gpu::GPUReconstruction::krnlRunRange const&, o2::gpu::GPUReconstruction::krnlEvent const&) (GPUChain.h:173)
==17471==    by 0x4AA343C: o2::gpu::GPUChainTracking::RunTPCTrackingMerger_Resolve(char, char, o2::gpu::GPUReconstruction::krnlDeviceType) (GPUChainTracking.cxx:1674)
==17471==    by 0x4AA4109: o2::gpu::GPUChainTracking::RunTPCTrackingMerger(bool) (GPUChainTracking.cxx:1745)
==17471==    by 0x4AA783C: o2::gpu::GPUChainTracking::RunChain() (GPUChainTracking.cxx:2111)
==17471==    by 0x4A74E4F: o2::gpu::GPUReconstructionCPU::RunChains() (GPUReconstructionCPU.cxx:212)
==17471==    by 0x139EE7: main (standalone.cxx:717)
==17471== 
==17471== Conditional jump or move depends on uninitialised value(s)
==17471==    at 0x141B1A: int const& std::max<int>(int const&, int const&) (stl_algobase.h:227)
==17471==    by 0x4A24B78: int o2::gpu::GPUCommonMath::Max<int>(int, int) (GPUCommonMath.h:280)
==17471==    by 0x4A3346E: o2::gpu::GPUTPCGMMerger::ResolveFindConnectedComponentsHook(int, int, int, int) (GPUTPCGMMerger.cxx:940)
==17471==    by 0x4A5A9FA: void o2::gpu::GPUTPCGMMergerResolve::Thread<1>(int, int, int, int, o2::gpu::GPUTPCGMMergerResolve::GPUSharedMemory&, o2::gpu::GPUTPCGMMerger&) (GPUTPCGMMergerGPU.cxx:78)
==17471==    by 0x4A7CD1F: int o2::gpu::GPUReconstructionCPUBackend::runKernelBackend<o2::gpu::GPUTPCGMMergerResolve, 1>(o2::gpu::GPUReconstruction::krnlSetup&) (GPUReconstructionCPU.cxx:83)
==17471==    by 0x4A79516: o2::gpu::GPUReconstructionKernels<o2::gpu::GPUReconstructionCPUBackend>::runKernelImpl(o2::gpu::GPUReconstruction::classArgument<o2::gpu::GPUTPCGMMergerResolve, 1>, o2::gpu::GPUReconstruction::krnlSetup&) (GPUReconstructionKernels.h:37)
==17471==    by 0x4AC951B: int o2::gpu::GPUReconstructionCPU::runKernel<o2::gpu::GPUTPCGMMergerResolve, 1, -1>(o2::gpu::GPUReconstruction::krnlExec const&, o2::gpu::GPUReconstruction::krnlRunRange const&, o2::gpu::GPUReconstruction::krnlEvent const&) (GPUReconstructionCPU.h:283)
==17471==    by 0x4AB353D: int o2::gpu::GPUChain::runKernel<o2::gpu::GPUTPCGMMergerResolve, 1, -1>(o2::gpu::GPUReconstruction::krnlExec const&, o2::gpu::GPUReconstruction::krnlRunRange const&, o2::gpu::GPUReconstruction::krnlEvent const&) (GPUChain.h:173)
==17471==    by 0x4AA343C: o2::gpu::GPUChainTracking::RunTPCTrackingMerger_Resolve(char, char, o2::gpu::GPUReconstruction::krnlDeviceType) (GPUChainTracking.cxx:1674)
==17471==    by 0x4AA4109: o2::gpu::GPUChainTracking::RunTPCTrackingMerger(bool) (GPUChainTracking.cxx:1745)
==17471==    by 0x4AA783C: o2::gpu::GPUChainTracking::RunChain() (GPUChainTracking.cxx:2111)
==17471==    by 0x4A74E4F: o2::gpu::GPUReconstructionCPU::RunChains() (GPUReconstructionCPU.cxx:212)
==17471== 
==17471== Conditional jump or move depends on uninitialised value(s)
==17471==    at 0x4A24A73: int const& std::min<int>(int const&, int const&) (stl_algobase.h:203)
==17471==    by 0x4A24B52: int o2::gpu::GPUCommonMath::Min<int>(int, int) (GPUCommonMath.h:274)
==17471==    by 0x4A33480: o2::gpu::GPUTPCGMMerger::ResolveFindConnectedComponentsHook(int, int, int, int) (GPUTPCGMMerger.cxx:941)
==17471==    by 0x4A5A9FA: void o2::gpu::GPUTPCGMMergerResolve::Thread<1>(int, int, int, int, o2::gpu::GPUTPCGMMergerResolve::GPUSharedMemory&, o2::gpu::GPUTPCGMMerger&) (GPUTPCGMMergerGPU.cxx:78)
==17471==    by 0x4A7CD1F: int o2::gpu::GPUReconstructionCPUBackend::runKernelBackend<o2::gpu::GPUTPCGMMergerResolve, 1>(o2::gpu::GPUReconstruction::krnlSetup&) (GPUReconstructionCPU.cxx:83)
==17471==    by 0x4A79516: o2::gpu::GPUReconstructionKernels<o2::gpu::GPUReconstructionCPUBackend>::runKernelImpl(o2::gpu::GPUReconstruction::classArgument<o2::gpu::GPUTPCGMMergerResolve, 1>, o2::gpu::GPUReconstruction::krnlSetup&) (GPUReconstructionKernels.h:37)
==17471==    by 0x4AC951B: int o2::gpu::GPUReconstructionCPU::runKernel<o2::gpu::GPUTPCGMMergerResolve, 1, -1>(o2::gpu::GPUReconstruction::krnlExec const&, o2::gpu::GPUReconstruction::krnlRunRange const&, o2::gpu::GPUReconstruction::krnlEvent const&) (GPUReconstructionCPU.h:283)
==17471==    by 0x4AB353D: int o2::gpu::GPUChain::runKernel<o2::gpu::GPUTPCGMMergerResolve, 1, -1>(o2::gpu::GPUReconstruction::krnlExec const&, o2::gpu::GPUReconstruction::krnlRunRange const&, o2::gpu::GPUReconstruction::krnlEvent const&) (GPUChain.h:173)
==17471==    by 0x4AA343C: o2::gpu::GPUChainTracking::RunTPCTrackingMerger_Resolve(char, char, o2::gpu::GPUReconstruction::krnlDeviceType) (GPUChainTracking.cxx:1674)
==17471==    by 0x4AA4109: o2::gpu::GPUChainTracking::RunTPCTrackingMerger(bool) (GPUChainTracking.cxx:1745)
==17471==    by 0x4AA783C: o2::gpu::GPUChainTracking::RunChain() (GPUChainTracking.cxx:2111)
==17471==    by 0x4A74E4F: o2::gpu::GPUReconstructionCPU::RunChains() (GPUReconstructionCPU.cxx:212)
==17471== 
Running kernel GPUTPCGMMergerResolve_step2 (Stream 0, Range 0/-1, Grid 1/1) on CPU
Running kernel GPUTPCGMMergerResolve_step3 (Stream 0, Range 0/-1, Grid 1/1) on CPU
==17471== Conditional jump or move depends on uninitialised value(s)
==17471==    at 0x4A3377D: o2::gpu::GPUTPCGMMerger::ResolveMergeSlices(o2::gpu::GPUTPCGMMerger::GPUResolveSharedMemory&, int, int, int, int, char, char) (GPUTPCGMMerger.cxx:1020)
==17471==    by 0x4A5AA8C: void o2::gpu::GPUTPCGMMergerResolve::Thread<3, char, char>(int, int, int, int, o2::gpu::GPUTPCGMMergerResolve::GPUSharedMemory&, o2::gpu::GPUTPCGMMerger&, char, char) (GPUTPCGMMergerGPU.cxx:90)
==17471==    by 0x4A7D224: int o2::gpu::GPUReconstructionCPUBackend::runKernelBackend<o2::gpu::GPUTPCGMMergerResolve, 3, char, char>(o2::gpu::GPUReconstruction::krnlSetup&, char const&, char const&) (GPUReconstructionCPU.cxx:83)
==17471==    by 0x4A795A3: o2::gpu::GPUReconstructionKernels<o2::gpu::GPUReconstructionCPUBackend>::runKernelImpl(o2::gpu::GPUReconstruction::classArgument<o2::gpu::GPUTPCGMMergerResolve, 3>, o2::gpu::GPUReconstruction::krnlSetup&, char, char) (GPUReconstructionKernels.h:39)
==17471==    by 0x4ACA22C: int o2::gpu::GPUReconstructionCPU::runKernel<o2::gpu::GPUTPCGMMergerResolve, 3, -1, char&, char&>(o2::gpu::GPUReconstruction::krnlExec const&, o2::gpu::GPUReconstruction::krnlRunRange const&, o2::gpu::GPUReconstruction::krnlEvent const&, char& const&, char& const&) (GPUReconstructionCPU.h:283)
==17471==    by 0x4AB35DB: int o2::gpu::GPUChain::runKernel<o2::gpu::GPUTPCGMMergerResolve, 3, -1, char&, char&>(o2::gpu::GPUReconstruction::krnlExec const&, o2::gpu::GPUReconstruction::krnlRunRange const&, o2::gpu::GPUReconstruction::krnlEvent const&, char&, char&) (GPUChain.h:173)
==17471==    by 0x4AA34D2: o2::gpu::GPUChainTracking::RunTPCTrackingMerger_Resolve(char, char, o2::gpu::GPUReconstruction::krnlDeviceType) (GPUChainTracking.cxx:1676)
==17471==    by 0x4AA4109: o2::gpu::GPUChainTracking::RunTPCTrackingMerger(bool) (GPUChainTracking.cxx:1745)
==17471==    by 0x4AA783C: o2::gpu::GPUChainTracking::RunChain() (GPUChainTracking.cxx:2111)
==17471==    by 0x4A74E4F: o2::gpu::GPUReconstructionCPU::RunChains() (GPUReconstructionCPU.cxx:212)
==17471==    by 0x139EE7: main (standalone.cxx:717)
==17471== 
==17471== Conditional jump or move depends on uninitialised value(s)
==17471==    at 0x4A33788: o2::gpu::GPUTPCGMMerger::ResolveMergeSlices(o2::gpu::GPUTPCGMMerger::GPUResolveSharedMemory&, int, int, int, int, char, char) (GPUTPCGMMerger.cxx:1020)
==17471==    by 0x4A5AA8C: void o2::gpu::GPUTPCGMMergerResolve::Thread<3, char, char>(int, int, int, int, o2::gpu::GPUTPCGMMergerResolve::GPUSharedMemory&, o2::gpu::GPUTPCGMMerger&, char, char) (GPUTPCGMMergerGPU.cxx:90)
==17471==    by 0x4A7D224: int o2::gpu::GPUReconstructionCPUBackend::runKernelBackend<o2::gpu::GPUTPCGMMergerResolve, 3, char, char>(o2::gpu::GPUReconstruction::krnlSetup&, char const&, char const&) (GPUReconstructionCPU.cxx:83)
==17471==    by 0x4A795A3: o2::gpu::GPUReconstructionKernels<o2::gpu::GPUReconstructionCPUBackend>::runKernelImpl(o2::gpu::GPUReconstruction::classArgument<o2::gpu::GPUTPCGMMergerResolve, 3>, o2::gpu::GPUReconstruction::krnlSetup&, char, char) (GPUReconstructionKernels.h:39)
==17471==    by 0x4ACA22C: int o2::gpu::GPUReconstructionCPU::runKernel<o2::gpu::GPUTPCGMMergerResolve, 3, -1, char&, char&>(o2::gpu::GPUReconstruction::krnlExec const&, o2::gpu::GPUReconstruction::krnlRunRange const&, o2::gpu::GPUReconstruction::krnlEvent const&, char& const&, char& const&) (GPUReconstructionCPU.h:283)
==17471==    by 0x4AB35DB: int o2::gpu::GPUChain::runKernel<o2::gpu::GPUTPCGMMergerResolve, 3, -1, char&, char&>(o2::gpu::GPUReconstruction::krnlExec const&, o2::gpu::GPUReconstruction::krnlRunRange const&, o2::gpu::GPUReconstruction::krnlEvent const&, char&, char&) (GPUChain.h:173)
==17471==    by 0x4AA34D2: o2::gpu::GPUChainTracking::RunTPCTrackingMerger_Resolve(char, char, o2::gpu::GPUReconstruction::krnlDeviceType) (GPUChainTracking.cxx:1676)
==17471==    by 0x4AA4109: o2::gpu::GPUChainTracking::RunTPCTrackingMerger(bool) (GPUChainTracking.cxx:1745)
==17471==    by 0x4AA783C: o2::gpu::GPUChainTracking::RunChain() (GPUChainTracking.cxx:2111)
==17471==    by 0x4A74E4F: o2::gpu::GPUReconstructionCPU::RunChains() (GPUReconstructionCPU.cxx:212)
==17471==    by 0x139EE7: main (standalone.cxx:717)

Could you have a look?

[fweig]

The crashes seems to be caused by your GetGridAuto patch. But i don't understand yet why this change breaks my code.

[davidrohr]

Oh, sorry, I should have checked that. But it seems quite odd to my. My patch shouldn't actually change anything. What will happen is that the CPU code should just get the blockcount of 1 instead of 4 (which you had before). But how could that break the code? The change affects step 3, but I get the valgrind error already in the previous steps. I could only imagine that breaks step 3 in a way that corrupts memory, and then yields to subsequent problems in the previous steps.

[fweig]

This is a weird one. I don't think your patch caused the bug. It just brought out what had been there before anyway, if it had any effect. The crashes appear randomly so maybe i was just (un)lucky when i rolled back the patch.
I've plastered the kernels with asserts but none of them trigger. And i don't see any crashes on gpu. Makes me think this might be caused by my atomic cas implmentation.

[davidrohr]

@fweig : This might even be a gcc problem or something strange.
The origin is indeed in your CPU cas implementation, that is why it doesn't trigger on GPU.
To be precise, it is in the openmp atomic capture, which doesn't work for some reason.
I compiled it without OpenMP, and then it seems to be ok.
That is probably also the reason you had to add the #pragma -Wno-unitiliazed. That seems to be exactly the warning for the uninitialized access...

[davidrohr]

I think it might be best to use builtin atomics instead of the OPENMP captures, which don't work well anyway. But it seems we would have to distinguish between gcc and clang:
https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
https://clang.llvm.org/docs/LanguageExtensions.html
And perhaps still use the OPENMP max/min for GCC.

I'll have a look tomorrow. If you have nothing to do tonight, feel free to give it a try :|

[davidrohr]

@fweig : So the problem is, you must not read the variable old inside the atomic clause, since it is the capture. Basically, old is only set at the end of the atomic clause, which is quite misleading.
*addr = (*addr == cmp) ? val : old; instead of *addr = (old == cmp) ? val : old; gives the correct result.
However, the openmp atomic seems to not work correctly with that semi-complicated term...
Baiscally, the comparison is not atomic, which breaks the algorithm.
The compiler would need to issue the lock cmpxchg assembler instruction, which he doesn't.
I have tried instead with __atomic_compare_exchange_4(&a, &b, c, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); and that works. I'll see what I can do about the other atomics, I'd prefer to get rid of the #pragma omp atomic and use GCC/Clang builtins instead, but unfortunately there seems to be no builtin for min/max.
I'll have a look and open a PR later today.

[davidrohr]

#3681 Should fix the problem with the atomics. Hopefully the builtins don't cause any trouble in the CI!

[davidrohr]

CI ran already