Heterogeneous Alpaka implementation of electron pixel seed-matching - #51200
Heterogeneous Alpaka implementation of electron pixel seed-matching#51200ckoraka wants to merge 10 commits into
Conversation
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51200/49727
|
|
A new Pull Request was created by @ckoraka for master. It involves the following packages:
@Moanwar, @cmsbuild, @jfernan2, @mandrenguyen, @srimanob can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
enable gpu |
|
@cmsbuild, please test |
|
assign heterogeneous |
| namespace ALPAKA_ACCELERATOR_NAMESPACE { | ||
| namespace reco { |
There was a problem hiding this comment.
could be simplified to
| namespace ALPAKA_ACCELERATOR_NAMESPACE { | |
| namespace reco { | |
| namespace ALPAKA_ACCELERATOR_NAMESPACE::reco { |
| } | ||
|
|
||
| template <typename TAcc> | ||
| ALPAKA_FN_ACC T partial_norm(const TAcc& acc) const { |
There was a problem hiding this comment.
partial_norm() and partial_norm2() are not very descriptive.
How about r() and r2() ?
There was a problem hiding this comment.
renamed to follow ROOT conventions, along with several other functions.
|
|
||
| Phys3DVector(const Phys3DVector<T>&) = default; | ||
|
|
||
| constexpr Phys3DVector(const T& value) : m_data{} { |
There was a problem hiding this comment.
probably better to pass it by value:
| constexpr Phys3DVector(const T& value) : m_data{} { | |
| constexpr Phys3DVector(const T value) : m_data{} { |
|
|
||
| namespace cms::alpakatools::math { | ||
|
|
||
| template <class T> |
There was a problem hiding this comment.
Can you add a short documentation, explaining that Phys3DVector represents a vector in 3D space in cartesian (x, y, z) or cylindrical (r, z) coordinates ?
There was a problem hiding this comment.
Also, how much more complicated would it be to use an Eigen::Matrix<T, 3, 1> here ?
It might give better performance due to how Eigen combines multiple operations.
There was a problem hiding this comment.
Eigen was attempted earlier, but that was hit by #48928
There was a problem hiding this comment.
A related bug has been reported to NVIDIA and fixed in CUDA 13.1, so this may have been resolved by now.
There was a problem hiding this comment.
Also, did anything change in the implementation ?
Now this looks like only Cartesian coordinates are used.
| }; | ||
|
|
||
| template <typename T> | ||
| inline constexpr Phys3DVector<T> ax(const T a, const Phys3DVector<T>& x) { |
| } | ||
|
|
||
| template <typename T> | ||
| inline constexpr Phys3DVector<T> xmy(const Phys3DVector<T>& x, const Phys3DVector<T>& y) { |
cmsbuild
added
tests-pending
code-checks-pending
and removed
tests-started
code-checks-approved
labels
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51200/49731
|
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51200/50480
|
|
Pull request #51200 was updated. @AdrianoDee, @DickyChant, @Martin-Grunewald, @Moanwar, @antoniovagnerini, @cmsbuild, @davidlange6, @fabiocos, @ftenchini, @fwyzard, @jfernan2, @kfjack, @makortel, @mandrenguyen, @miquork, @mmusich, @srimanob, @sroychow can you please check and sign again. |
Added with commit 863c01f |
|
test parameters:
|
|
@cmsbuild, please test |
|
-1 Failed Tests: UnitTests RelVals-AMD_MI300X Failed Unit TestsI found 1 errors in the following unit tests: ---> test test_heterogeneousMenu had ERRORS Failed RelVals-AMD_MI300X
Comparison SummarySummary:
AMD_W7900 Comparison SummaryThere are some workflows for which there are errors in the baseline: Summary:
NVIDIA_H100 Comparison SummarySummary:
NVIDIA_L40S Comparison SummarySummary:
NVIDIA_T4 Comparison SummarySummary:
Max Memory Comparisons exceeding threshold@cms-sw/core-l2 , I found 73 workflow step(s) with memory usage exceeding the error threshold: Expand to see workflows ...
Max Memory Comparisons exceeding threshold NVIDIA_H100@cms-sw/core-l2 , I found 2 workflow step(s) with memory usage exceeding the error threshold: Expand to see workflows ...
Max Memory Comparisons exceeding threshold NVIDIA_L40S@cms-sw/core-l2 , I found 2 workflow step(s) with memory usage exceeding the error threshold: Expand to see workflows ...
Max Memory Comparisons exceeding threshold NVIDIA_T4@cms-sw/core-l2 , I found 2 workflow step(s) with memory usage exceeding the error threshold: Expand to see workflows ...
|
the failure in this test is genuine.
? 
Footnotes
|
|
test parameters: |
|
@cmsbuild, please build To have a ROOT debug build with this PR included in order to continue hunting types whose dictionary being missing triggers ROOT header parsing in connection to this PR. |
|
+1 Size: This PR adds an extra 40KB to repository The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: You can see more details here: |
|
please test Hopefully the AMD tests should succeed now. |
fwyzard
left a comment
fwyzard
left a comment
There was a problem hiding this comment.
Apologies for taking so long - I'm finally through with the review.
Other than some suggestions to clean up the code, the main point is to make the code more generic, so float and double can be used more easily.
Based on the latest presentation I think there is in fact good reason for moving to float in most of the code.
| // Helper function to compute relative position | ||
| template <typename T> | ||
| constexpr auto relativePosition(const math::Phys3DVector<T>& point, const math::Phys3DVector<T>& origin) | ||
| -> math::Phys3DVector<T> { | ||
| return point - origin; | ||
| } |
There was a problem hiding this comment.
IMHO this could just be replaced by point - origin at the call site.
|
|
||
| if (nrm == 0.) | ||
| if (mag == 0.) |
| constexpr Phys3DVector(const T value) : m_data{} { | ||
| CMS_UNROLL_LOOP | ||
| for (int i = 0; i < 3; i++) { | ||
| m_data[i] = value; | ||
| } |
There was a problem hiding this comment.
| constexpr Phys3DVector(const T value) : m_data{} { | |
| CMS_UNROLL_LOOP | |
| for (int i = 0; i < 3; i++) { | |
| m_data[i] = value; | |
| } | |
| constexpr Phys3DVector(const T value) : m_data{value, value, value} {} |
to avoid the initialisation followed by the assignments (even if they should be optimised).
| constexpr Phys3DVector(const T x, const T y, const T z) : m_data{} { | ||
| m_data[0] = x; | ||
| m_data[1] = y; | ||
| m_data[2] = z; | ||
| } |
There was a problem hiding this comment.
| constexpr Phys3DVector(const T x, const T y, const T z) : m_data{} { | |
| m_data[0] = x; | |
| m_data[1] = y; | |
| m_data[2] = z; | |
| } | |
| constexpr Phys3DVector(const T x, const T y, const T z) : m_data{x, y, z} {} |
| inline constexpr void zero() { | ||
| CMS_UNROLL_LOOP | ||
| for (int i = 0; i < 3; i++) { | ||
| m_data[i] = static_cast<T>(0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Given that there are exactly three elements, I would find it more clear to unroll the loops explicitly:
| inline constexpr void zero() { | |
| CMS_UNROLL_LOOP | |
| for (int i = 0; i < 3; i++) { | |
| m_data[i] = static_cast<T>(0); | |
| } | |
| } | |
| inline constexpr void zero() { | |
| m_data[0] = static_cast<T>(0); | |
| m_data[1] = static_cast<T>(0); | |
| m_data[2] = static_cast<T>(0); | |
| } |
Same for all other methods.
| reco::SuperClusterDeviceCollection deviceProductSCs{event.queue(), superClusterCollectionSize}; | ||
| reco::ElectronSeedDeviceCollection deviceProductSeeds{event.queue(), seedCollectionSize}; | ||
| alpaka::memcpy(event.queue(), deviceProductSCs.buffer(), hostProductSCs.buffer()); | ||
| alpaka::memcpy(event.queue(), deviceProductSeeds.buffer(), hostProductSeeds.buffer()); |
There was a problem hiding this comment.
The copy could be avoided when the back-end is the CPU.
| template <typename TAcc, typename = std::enable_if_t<alpaka::concepts::Acc<TAcc>>> | ||
| ALPAKA_FN_ACC void operator()(TAcc const& acc, |
There was a problem hiding this comment.
Can this be simply Acc1D ?
| template <typename TAcc, typename = std::enable_if_t<alpaka::concepts::Acc<TAcc>>> | |
| ALPAKA_FN_ACC void operator()(TAcc const& acc, | |
| ALPAKA_FN_ACC void operator()(Acc1D const& acc, |
| // Constructor to compute relative points | ||
| constexpr EleRelPointPairPortable(const Vec3& p1, const Vec3& p2, const Vec3& origin) | ||
| : relP1(relativePosition(p1, origin)), relP2(relativePosition(p2, origin)) {} | ||
|
|
There was a problem hiding this comment.
Can you add a deduction guide, so EleRelPointPairPortable pair(p1, p2, origin) can deduce the type of T from that of p1, p2, origin ?
| continue; | ||
|
|
||
| const double zVertex = | ||
| getZVtxFromExtrapolation<TAcc, typename Vec3d::value_type>(acc, vertex, hitPosition, positionSC); |
There was a problem hiding this comment.
TAcc can be deduced from acc.
Can Vec3d::value_type be deduced from the arguments?
|
|
||
| const double zVertex = | ||
| getZVtxFromExtrapolation<TAcc, typename Vec3d::value_type>(acc, vertex, hitPosition, positionSC); | ||
| Vec3d vertexUpdated(vertex[0], vertex[1], zVertex); |
There was a problem hiding this comment.
At a glance vertex is not used any more, so it could be updated in place ?
|
please test for el9_amd64_gcc13 To clean the old results. |
|
-heterogeneous To remove it from the queue, while the comments are being addressed. |
|
@fwyzard reading though your comments, I understand that the conclusion now is to template the floating point type essentially everywhere in the code of this branch / PR instead of just using single precision If so, should I still run performance comparisons (execution time, (GPU) memory consumption) for |
|
If we decide to use If we need to keep |
Yes, I think that would be interesting
I don't know what kind of information is provided by the setup on the timing server.
I guess that depends on what workflow you run. But it's true that for Phase-2 studies the timing server is not the best setup, for various reasons. |
|
Thanks @fwyzard . I'll check which machine to use for the studies. |
9 participants
PR description:
This PR introduces a heterogeneous implementation of the electron pixel seed-matching algorithm, intended to replace the legacy ElectronNHitSeedProducer. It introduces the following developments:
Two new SoA data formats in DataFormats/EgammaReco:
Portable utility functions for helix track propagation, ported to be usable on device: helixBarrelPlaneCrossingByCircle, helixForwardPlaneCrossing, helixArbitraryPlaneCrossing(and 2nd order), ftsFromVertexToPointPortable, EleRelPointPairPortable, and Plane.
A dedicated Alpaka kernel that performs the seed-to-supercluster matching on the device.
Two new producers implementing the new workflow:
PR validation:
Timing and performance studies have been conducted to validate these developments. The results can be found here:
Note One can run these developments using :
runTheMatrix.py -w upgrade -l 34434.751adding a customization to step2 :
--customise RecoEgamma/Configuration/hltEgammaPixelSeedingPortable.customise_alpaka_seedsor using :
runTheMatrix.py -w upgrade -l 34434.7511