SoA backend cleanup#50131
Conversation
|
type ngt |
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-50131/48054
|
|
A new Pull Request was created by @Electricks94 for master. It involves the following packages:
@cmsbuild, @fwyzard, @hjkwon260, @makortel, @valsdav, @y19y19 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| soa_impl_os << " Scalar " << name << " at offset " << soa_impl_offset << " has size " << sizeof(T) | ||
| << " and padding " << ((sizeof(T) - 1) / alignment + 1) * alignment - sizeof(T) << std::endl; | ||
| soa_impl_offset += ((sizeof(T) - 1) / alignment + 1) * alignment; | ||
| SOA_INLINE bool checkAlignment(const T* addr, byte_size_type alignment) { |
There was a problem hiding this comment.
I just noticed that all SOA_INLINE I could find in cmssw are preceded by a SOA_HOST_DEVICE. Is there any reason why SOA_HOST_DEVICE is not used here?
There was a problem hiding this comment.
Inlining functions that are also executed on the device is a very common case in the SoA backend. Here, however, I don't really see a usecase to check the alignment on the device side. Hence, I left out SOA_HOST_DEVICE
|
|
||
| namespace cms::soa::detail { | ||
| // Helper function for streaming column | ||
| // Helper function to check alignment of a pointer. Returns true if the pointer is not aligned to the specified alignment. |
There was a problem hiding this comment.
Since checkAlignment returns true if the pointer is not aligned, should it better be named checkMisaligned, checkMisalignment, isMisaligned, etc.?
or it could return true when the pointer is aligned instead.
There was a problem hiding this comment.
I think isMisaligned is the most suitable name. I adapted it
Electricks94
force-pushed
the
SoAClean
branch
from
0867b48 to
a2967db
Compare
There was a problem hiding this comment.
please add
#include <alpaka/alpaka.hpp>when you use directly alpaka functions.
| , \ | ||
| /* Eigen column */ \ | ||
| if (not readyToSet) { \ | ||
| if (!readyToSet) { \ |
There was a problem hiding this comment.
Please keep the not naming.
There was a problem hiding this comment.
Because it is clearer to read or is there a reason I oversee?
There was a problem hiding this comment.
Because it is more expressive and clearer to read.
| } \ | ||
| constexpr static cms::soa::SoAColumnType BOOST_PP_CAT(ColumnTypeOf_, NAME) = cms::soa::SoAColumnType::eigen; \ | ||
| ) \ | ||
| ) \ |
There was a problem hiding this comment.
please keep the indentation lined up
Electricks94
force-pushed
the
SoAClean
branch
from
a2967db to
28c0c56
Compare
|
is it practical to add these definitions in lst.cc file in this PR cmssw/RecoTracker/LSTCore/standalone/bin/lst.cc Lines 565 to 570 in d905322 or should we followup with a separate PR? |
Right, throwing an exception generates quite a bit of machine code, so this way the dependent shared libraries can be kept a little bit smaller. |
|
While my preference would be to link the library form CMSSW,
yes, this is also a solution. @Electricks94 can you copy-paste the content of |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-50131/48300
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
Co-authored-by: Andrea Bocci <fwyzard@gmail.com>
|
Pull request #50131 was updated. @Moanwar, @civanch, @cmsbuild, @fwyzard, @hjkwon260, @jfernan2, @kpedro88, @makortel, @mandrenguyen, @mdhildreth, @srimanob, @valsdav, @y19y19 can you please check and sign again. |
|
+heterogenous |
|
test parameters:
|
|
please test |
|
-1 Failed Tests: nvidia_l40sUnitTests Comparison SummarySummary:
AMD_MI300X Comparison SummarySummary:
AMD_W7900 Comparison SummarySummary:
NVIDIA_H100 Comparison SummarySummary:
NVIDIA_L40S Comparison SummarySummary:
NVIDIA_T4 Comparison SummarySummary:
|
I tried to reproduce the error locally on the NGT Farm but the test case did not produce any errors. Could you please rerun the test case so that I can see the error message in the pipeline? |
|
sure |
|
please test |
|
+1 Size: This PR adds an extra 16KB to repository Comparison SummarySummary:
AMD_MI300X Comparison SummarySummary:
AMD_W7900 Comparison SummarySummary:
NVIDIA_H100 Comparison SummarySummary:
NVIDIA_L40S Comparison SummarySummary:
NVIDIA_T4 Comparison SummarySummary:
|
|
+heterogeneous |
|
urgent I would like to have this in 16.1.0-pre3, if possible. @cms-sw/ml-l2 @cms-sw/reconstruction-l2 @cms-sw/simulation-l2 could you review the changes, and eventually sign them off, please ? |
|
+ml |
|
+1 |
1 similar comment
|
+1 |
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @ftenchini, @sextonkennedy, @mandrenguyen (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
10 participants
This PR introduces various cleanups to the SoA backend:
TupleOrPointerTypeinSoAParametersImpl.TupleOrPointerTypeis replaced by member functions that return the data address (and the stride in the case of an Eigen Column).checkAlignmentfunction inSoAParametersImpl. The function was mostly code duplication across the four specializations ofSoAParametersImpl. It is now a free function in thecms::soa::detailnamespace.AccumulateColumnByteSizesandcomputePitchwere essentially code duplication. It is replaced byComputePitchwhich is a rename ofAccumulateColumnByteSizesand replaces the functioncomputePitchentirely.printColumnis implemented as aPrintColumnstruct nowcheckAlignmentfunction_DECLARE_VIEW_CONSTRUCTION_BYCOLUMN_PARAMETERSand_DECLARE_VIEW_MEMBER_INITIALIZERS_BYCOLUMNmacrosSoALayoutby a test case that actually dumps an SoA and checks the output string_deepCopywas implemented inPortableDeviceCollectionandPortableHostCollectionwith the exact same functionality. It is not moved to theportablecollectionnamespace and called in both places.Fyi @felicepantaleo