Add support to multi layout portable collections (any number) #40285
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40285/33338
|
|
A new Pull Request was created by @ericcano (Eric Cano) for master. It involves the following packages:
@cmsbuild, @makortel, @fwyzard can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| f(std::integral_constant<decltype(Start), Start>()); | ||
| constexpr_for<Start + Inc, End, Inc>(f); |
There was a problem hiding this comment.
I'm just wondering if these should be
| f(std::integral_constant<decltype(Start), Start>()); | |
| constexpr_for<Start + Inc, End, Inc>(f); | |
| std::forward<F>(f)(std::integral_constant<decltype(Start), Start>()); | |
| constexpr_for<Start + Inc, End, Inc>(std::move(f)); |
to support rvalue f ?
There was a problem hiding this comment.
Also, I would use std::size_t explicitly instead of auto, and give a default value of Inc = 1
| } | ||
| } | ||
|
|
||
| template <std::size_t idx, typename T> |
There was a problem hiding this comment.
can you change idx to Idx ?
| template <std::size_t idx, typename T> | |
| template <std::size_t Idx, typename T> |
| template <std::size_t idx, typename T> | ||
| struct CollectionLeaf { | ||
| CollectionLeaf() = default; | ||
| CollectionLeaf(std::byte* buffer, int32_t elements) : layout_(buffer, elements), view_(layout_) {} |
There was a problem hiding this comment.
I would wither change this to
| CollectionLeaf(std::byte* buffer, int32_t elements) : layout_(buffer, elements), view_(layout_) {} | |
| CollectionLeaf(std::byte* buffer, int32_t size) : layout_(buffer, size), view_(layout_) {} |
or use elements instead of sizes in the next one
| CollectionLeaf() = default; | ||
| CollectionLeaf(std::byte* buffer, int32_t elements) : layout_(buffer, elements), view_(layout_) {} | ||
| template <std::size_t N> | ||
| CollectionLeaf(std::byte* buffer, std::array<int32_t, N> sizes) : layout_(buffer, sizes[idx]), view_(layout_) { |
There was a problem hiding this comment.
would it be better to pass the array as a const reference ?
| CollectionLeaf(std::byte* buffer, std::array<int32_t, N> sizes) : layout_(buffer, sizes[idx]), view_(layout_) { | |
| CollectionLeaf(std::byte* buffer, std::array<int32_t, N> const& sizes) : layout_(buffer, sizes[idx]), view_(layout_) { |
|
|
||
| template <typename T0, typename T1, typename T2, typename T3, typename T4> | ||
| struct CollectionTypeResolver { | ||
| template <std::size_t idx, class = void> |
There was a problem hiding this comment.
| template <std::size_t idx, class = void> | |
| template <std::size_t idx, typename = void> |
for consistency
| template <typename T0, typename T1, typename T2, typename T3, typename T4> | ||
| struct CollectionTypeResolver { | ||
| template <std::size_t idx, class = void> | ||
| struct Resolver { | ||
| static_assert(idx != 0); | ||
| using type = typename CollectionTypeResolver<T1, T2, T3, T4, void>::template Resolver<idx - 1>::type; | ||
| }; | ||
|
|
||
| template <std::size_t idx> | ||
| struct Resolver<idx, std::enable_if_t<idx == 0>> { | ||
| using type = T0; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
could this all be replaced by std::tuple_element<Idx, std::tuple<T0, T1, T2, T3, T4>> ?
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40285/33391
|
|
please test |
|
I'm working on some more suggestions, but let's see if these work first. |
|
-1 Failed Tests: ClangBuild Clang BuildI found compilation warning while trying to compile with clang. Command used: See details on the summary page. |
| std::array<int32_t, members_> sizes; | ||
| constexpr_for<0, members_>( | ||
| [&sizes, &impl](auto i) { sizes[i] = static_cast<Leaf<i> const&>(impl).layout_.metadata().size(); }); | ||
| new (newObj) PortableHostCollection(sizes, cms::alpakatools::host()); | ||
| constexpr_for<0, members_>([&sizes, &newObj, &impl](auto i) { | ||
| static_cast<Leaf<i>&>(newObj->impl_).layout_.ROOTReadStreamer(static_cast<Leaf<i> const&>(impl).layout_); | ||
| }); |
There was a problem hiding this comment.
@ericcano sizes does not seem to be used here; do we need it ?
There was a problem hiding this comment.
Indeed, it is not needed for the second constexpr_for loop on line 220.
ericcano
force-pushed
the
multiCollections
branch
from
6b07af8
to
b4ed3a8
Compare
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40285/38743
|
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-72ad56/37270/summary.html Comparison SummarySummary:
|
|
With these changes, the root file produced by $ edmDumpEventContent --forceColumns test.root
Type Module Label Process
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
PortableHostCollection<portabletest::TestSoALayout<128,false> > "testProducer" "" "Writer"
PortableHostCollection<portabletest::TestSoALayout<128,false> > "testProducerSerial" "" "Writer"
PortableHostMultiCollection<portabletest::TestSoALayout<128,false>,portabletest::TestSoALayout2<128,false> > "testProducer" "" "Writer"
PortableHostMultiCollection<portabletest::TestSoALayout<128,false>,portabletest::TestSoALayout2<128,false> > "testProducerSerial" "" "Writer"
PortableHostMultiCollection<portabletest::TestSoALayout<128,false>,portabletest::TestSoALayout2<128,false>,portabletest::TestSoALayout3<128,false> > "testProducer" "" "Writer"
PortableHostMultiCollection<portabletest::TestSoALayout<128,false>,portabletest::TestSoALayout2<128,false>,portabletest::TestSoALayout3<128,false> > "testProducerSerial" "" "Writer"
PortableHostObject<portabletest::TestStruct> "testProducer" "" "Writer"
PortableHostObject<portabletest::TestStruct> "testProducerSerial" "" "Writer"
edm::TriggerResults "TriggerResults" "" "Writer"
unsigned short "testProducer" "backend" "Writer"
unsigned short "testProducerSerial" "backend" "Writer" |
|
+heterogeneous |
cmsbuild
added
fully-signed
heterogeneous-approved
and removed
pending-signatures
heterogeneous-pending
labels
|
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. @rappoccio, @sextonkennedy, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
The design is recursive, but ROOT seems not to serialize variadic templates, so we have a 5 elements design. This number is arbitrary and can be changed with relatively little work.
The order of the PortableDeviceCollection template parameters is changed from device at the end to device at the begining to accomodate the pseudo variadic templation.
A test validates the colleciton with two layouts.
The layout and view can be accessed by index or type, on the condition the type is present only once in the collection.
The default index is set to 0 to minimize the need for chages in the single layout case. Empty template parameters square bracket ("<>") are needed in some cases.
PR validation:
The PR is validated with the pre-existing
/HeterogeneousCore/AlpakaTest/test/testHeterogeneousCoreAlpakaTestWriteRead.shwhich has been extended to add a two-layouts example.The rest of CMSSW needs to be reviewed to detect where the PortableCollections are used to validate if syntactic adaptations are needed (mostly adding
<>to functions that became templated, if needed. For this reason, this PR is a draft.