From c30702ccf4a45d4ede4ae9d553acb2ab0324b612 Mon Sep 17 00:00:00 2001 From: Matti Kortelainen Date: Fri, 19 Apr 2024 23:23:20 +0200 Subject: [PATCH 1/4] Add tests and rudimentary protections for default-constructed PortableCollections Also add tests for zero-sized PortableCollections --- .../Portable/interface/PortableCollection.h | 6 + .../interface/PortableDeviceCollection.h | 56 ++++++-- .../interface/PortableHostCollection.h | 56 ++++++-- DataFormats/Portable/test/BuildFile.xml | 9 ++ .../Portable/test/alpaka/test_catch2_main.cc | 2 + .../test_catch2_portableCollection.dev.cc | 121 ++++++++++++++++++ .../test_catch2_portableHostCollection.cc | 53 ++++++++ DataFormats/SoATemplate/test/SoAUnitTests.cc | 16 +++ 8 files changed, 299 insertions(+), 20 deletions(-) create mode 100644 DataFormats/Portable/test/alpaka/test_catch2_main.cc create mode 100644 DataFormats/Portable/test/alpaka/test_catch2_portableCollection.dev.cc create mode 100644 DataFormats/Portable/test/test_catch2_portableHostCollection.cc diff --git a/DataFormats/Portable/interface/PortableCollection.h b/DataFormats/Portable/interface/PortableCollection.h index abfffff6ed1d2..713c7a6084483 100644 --- a/DataFormats/Portable/interface/PortableCollection.h +++ b/DataFormats/Portable/interface/PortableCollection.h @@ -50,6 +50,9 @@ namespace cms::alpakatools { struct CopyToHost> { template static auto copyAsync(TQueue& queue, PortableDeviceCollection const& srcData) { + if (not srcData.isValid()) { + return PortableHostCollection(); + } PortableHostCollection dstData(srcData->metadata().size(), queue); alpaka::memcpy(queue, dstData.buffer(), srcData.buffer()); return dstData; @@ -71,6 +74,9 @@ namespace cms::alpakatools { template static auto copyAsync(TQueue& queue, PortableHostCollection const& srcData) { using TDevice = typename alpaka::trait::DevType::type; + if (not srcData.isValid()) { + return PortableDeviceCollection(); + } PortableDeviceCollection dstData(srcData->metadata().size(), queue); alpaka::memcpy(queue, dstData.buffer(), srcData.buffer()); return dstData; diff --git a/DataFormats/Portable/interface/PortableDeviceCollection.h b/DataFormats/Portable/interface/PortableDeviceCollection.h index 38e1c90c9a37d..ef97122504172 100644 --- a/DataFormats/Portable/interface/PortableDeviceCollection.h +++ b/DataFormats/Portable/interface/PortableDeviceCollection.h @@ -54,21 +54,57 @@ class PortableDeviceCollection { // default destructor ~PortableDeviceCollection() = default; + // has a valid buffer? + bool isValid() const { return buffer_.has_value(); } + + // a way to access the number of elements without the buffer validity assertion + auto size() const { return view_.metadata().size(); } + // access the View - View& view() { return view_; } - ConstView const& view() const { return view_; } - ConstView const& const_view() const { return view_; } + View& view() { + assert(isValid()); + return view_; + } + ConstView const& view() const { + assert(isValid()); + return view_; + } + ConstView const& const_view() const { + assert(isValid()); + return view_; + } - View& operator*() { return view_; } - ConstView const& operator*() const { return view_; } + View& operator*() { + assert(isValid()); + return view_; + } + ConstView const& operator*() const { + assert(isValid()); + return view_; + } - View* operator->() { return &view_; } - ConstView const* operator->() const { return &view_; } + View* operator->() { + assert(isValid()); + return &view_; + } + ConstView const* operator->() const { + assert(isValid()); + return &view_; + } // access the Buffer - Buffer buffer() { return *buffer_; } - ConstBuffer buffer() const { return *buffer_; } - ConstBuffer const_buffer() const { return *buffer_; } + Buffer buffer() { + assert(isValid()); + return *buffer_; + } + ConstBuffer buffer() const { + assert(isValid()); + return *buffer_; + } + ConstBuffer const_buffer() const { + assert(isValid()); + return *buffer_; + } private: std::optional buffer_; //! diff --git a/DataFormats/Portable/interface/PortableHostCollection.h b/DataFormats/Portable/interface/PortableHostCollection.h index 61dde4c58f425..f21deb45bba4e 100644 --- a/DataFormats/Portable/interface/PortableHostCollection.h +++ b/DataFormats/Portable/interface/PortableHostCollection.h @@ -53,21 +53,57 @@ class PortableHostCollection { // default destructor ~PortableHostCollection() = default; + // has a valid buffer? + bool isValid() const { return buffer_.has_value(); } + + // a way to access the number of elements without the buffer validity assertion + auto size() const { return view_.metadata().size(); } + // access the View - View& view() { return view_; } - ConstView const& view() const { return view_; } - ConstView const& const_view() const { return view_; } + View& view() { + assert(isValid()); + return view_; + } + ConstView const& view() const { + assert(isValid()); + return view_; + } + ConstView const& const_view() const { + assert(isValid()); + return view_; + } - View& operator*() { return view_; } - ConstView const& operator*() const { return view_; } + View& operator*() { + assert(isValid()); + return view_; + } + ConstView const& operator*() const { + assert(isValid()); + return view_; + } - View* operator->() { return &view_; } - ConstView const* operator->() const { return &view_; } + View* operator->() { + assert(isValid()); + return &view_; + } + ConstView const* operator->() const { + assert(isValid()); + return &view_; + } // access the Buffer - Buffer buffer() { return *buffer_; } - ConstBuffer buffer() const { return *buffer_; } - ConstBuffer const_buffer() const { return *buffer_; } + Buffer buffer() { + assert(isValid()); + return *buffer_; + } + ConstBuffer buffer() const { + assert(isValid()); + return *buffer_; + } + ConstBuffer const_buffer() const { + assert(isValid()); + return *buffer_; + } // part of the ROOT read streamer static void ROOTReadStreamer(PortableHostCollection* newObj, Layout& layout) { diff --git a/DataFormats/Portable/test/BuildFile.xml b/DataFormats/Portable/test/BuildFile.xml index 25a83ba4eebe0..104a9fe0a3255 100644 --- a/DataFormats/Portable/test/BuildFile.xml +++ b/DataFormats/Portable/test/BuildFile.xml @@ -3,3 +3,12 @@ + + + + + + + + + diff --git a/DataFormats/Portable/test/alpaka/test_catch2_main.cc b/DataFormats/Portable/test/alpaka/test_catch2_main.cc new file mode 100644 index 0000000000000..b3143fbb1788b --- /dev/null +++ b/DataFormats/Portable/test/alpaka/test_catch2_main.cc @@ -0,0 +1,2 @@ +#define CATCH_CONFIG_MAIN +#include diff --git a/DataFormats/Portable/test/alpaka/test_catch2_portableCollection.dev.cc b/DataFormats/Portable/test/alpaka/test_catch2_portableCollection.dev.cc new file mode 100644 index 0000000000000..0a6977b3d8af4 --- /dev/null +++ b/DataFormats/Portable/test/alpaka/test_catch2_portableCollection.dev.cc @@ -0,0 +1,121 @@ +#include + +#include "DataFormats/Portable/interface/PortableCollection.h" +#include "DataFormats/SoATemplate/interface/SoACommon.h" +#include "DataFormats/SoATemplate/interface/SoALayout.h" +#include "DataFormats/SoATemplate/interface/SoAView.h" +#include "FWCore/Utilities/interface/stringize.h" +#include "HeterogeneousCore/AlpakaInterface/interface/config.h" +#include "HeterogeneousCore/AlpakaInterface/interface/workdivision.h" + +// each test binary is built for a single Alpaka backend +using namespace ALPAKA_ACCELERATOR_NAMESPACE; + +namespace { + GENERATE_SOA_LAYOUT(TestLayout, SOA_COLUMN(double, x), SOA_COLUMN(int32_t, id), SOA_SCALAR(uint32_t, num)) + + using TestSoA = TestLayout<>; + + constexpr auto s_tag = "[PortableCollection]"; +} // namespace + +TEST_CASE("PortableCollection", s_tag) { + // get the list of devices on the current platform + auto const& devices = cms::alpakatools::devices(); + if (devices.empty()) { + FAIL("No devices available for the " EDM_STRINGIZE(ALPAKA_ACCELERATOR_NAMESPACE) " backend, " + "the test will be skipped."); + } + + SECTION("Default constructor") { + PortableHostCollection coll_h; + REQUIRE(coll_h.size() == 0); + REQUIRE(not coll_h.isValid()); + + // Following lines would be undefined behavior, and could lead to crashes + //coll->num() = 42; + //REQUIRE(coll->num() == 42); + + // CopyToDevice> is not defined +#ifndef ALPAKA_ACC_CPU_B_SEQ_T_SEQ_ENABLED + for (auto const& device : devices) { + auto queue = Queue(device); + auto coll_d = cms::alpakatools::CopyToDevice>::copyAsync(queue, coll_h); + REQUIRE(coll_d.size() == 0); + REQUIRE(not coll_d.isValid()); + alpaka::wait(queue); + } +#endif + } + + SECTION("Zero size") { + int constexpr size = 0; + PortableHostCollection coll_h(size, cms::alpakatools::host()); + REQUIRE(coll_h.isValid()); + REQUIRE(coll_h->metadata().size() == size); + coll_h->num() = 42; + + // CopyToDevice> is not defined +#ifdef ALPAKA_ACC_CPU_B_SEQ_T_SEQ_ENABLED + REQUIRE(coll_h->num() == 42); +#else + for (auto const& device : devices) { + auto queue = Queue(device); + auto coll_d = cms::alpakatools::CopyToDevice>::copyAsync(queue, coll_h); + REQUIRE(coll_d.isValid()); + REQUIRE(coll_d.size() == size); + + auto div = cms::alpakatools::make_workdiv(1, 1); + alpaka::exec( + queue, + div, + [] ALPAKA_FN_ACC(Acc1D const& acc, TestSoA::ConstView view) { + assert(view.metadata().size() == size); + assert(view.num() == 42); + }, + coll_d.const_view()); + alpaka::wait(queue); + } +#endif + } + + SECTION("Non-zero size") { + int constexpr size = 10; + PortableHostCollection coll_h(size, cms::alpakatools::host()); + REQUIRE(coll_h.isValid()); + coll_h->num() = 20; + + for (int i = 0; i < size; ++i) { + coll_h->id(i) = i * 2 + 1; + } + + // CopyToDevice> is not defined +#ifdef ALPAKA_ACC_CPU_B_SEQ_T_SEQ_ENABLED + for (int i = 0; i < size; ++i) { + assert(coll_h->id(i) == i * 2 + 1); + } +#else + for (auto const& device : devices) { + auto queue = Queue(device); + auto coll_d = cms::alpakatools::CopyToDevice>::copyAsync(queue, coll_h); + REQUIRE(coll_d.isValid()); + REQUIRE(coll_d.size() == size); + + auto div = cms::alpakatools::make_workdiv(1, size); + alpaka::exec( + queue, + div, + [] ALPAKA_FN_ACC(Acc1D const& acc, TestSoA::ConstView view) { + assert(view.metadata().size() == size); + assert(view.num() == 20); + for (int i : cms::alpakatools::uniform_elements(acc)) { + assert(view.id(i) == i * 2 + 1); + } + }, + coll_d.const_view()); + + alpaka::wait(queue); + } +#endif + } +} diff --git a/DataFormats/Portable/test/test_catch2_portableHostCollection.cc b/DataFormats/Portable/test/test_catch2_portableHostCollection.cc new file mode 100644 index 0000000000000..6eb6e640b3ea1 --- /dev/null +++ b/DataFormats/Portable/test/test_catch2_portableHostCollection.cc @@ -0,0 +1,53 @@ +#include + +#include "DataFormats/Portable/interface/PortableHostCollection.h" +#include "DataFormats/SoATemplate/interface/SoACommon.h" +#include "DataFormats/SoATemplate/interface/SoALayout.h" +#include "DataFormats/SoATemplate/interface/SoAView.h" + +namespace { + GENERATE_SOA_LAYOUT(TestLayout, SOA_COLUMN(double, x), SOA_COLUMN(int32_t, id), SOA_SCALAR(uint32_t, num)) + + using TestSoA = TestLayout<>; + + constexpr auto s_tag = "[PortableHostCollection]"; +} // namespace + +TEST_CASE("PortableHostCollection", s_tag) { + SECTION("Default constructor") { + PortableHostCollection coll; + REQUIRE(coll.size() == 0); + REQUIRE(not coll.isValid()); + + // Following lines would be undefined behavior, and could lead to crashes + //coll->num() = 42; + //REQUIRE(coll->num() == 42); + } + + SECTION("Zero size") { + int constexpr size = 0; + PortableHostCollection coll(size, cms::alpakatools::host()); + REQUIRE(coll.size() == size); + REQUIRE(coll.isValid()); + + coll->num() = 42; + REQUIRE(coll->num() == 42); + } + + SECTION("Non-zero size") { + int constexpr size = 10; + PortableHostCollection coll(size, cms::alpakatools::host()); + REQUIRE(coll.size() == size); + REQUIRE(coll.isValid()); + + coll->num() = 42; + for (int i = 0; i < size; ++i) { + coll->id()[i] = i * 2; + } + + REQUIRE(coll->num() == 42); + for (int i = 0; i < size; ++i) { + REQUIRE(coll->id()[i] == i * 2); + } + } +} diff --git a/DataFormats/SoATemplate/test/SoAUnitTests.cc b/DataFormats/SoATemplate/test/SoAUnitTests.cc index b0c27f85a3322..ff91dde85b12d 100644 --- a/DataFormats/SoATemplate/test/SoAUnitTests.cc +++ b/DataFormats/SoATemplate/test/SoAUnitTests.cc @@ -11,11 +11,27 @@ GENERATE_SOA_LAYOUT(SimpleLayoutTemplate, SOA_COLUMN(float, y), SOA_COLUMN(float, z), SOA_COLUMN(float, t)) + +GENERATE_SOA_LAYOUT(SimpleLayoutTemplateWithScalar, + SOA_COLUMN(float, x), + SOA_SCALAR(unsigned int, s)) // clang-format on using SimpleLayout = SimpleLayoutTemplate<>; TEST_CASE("SoATemplate") { + SECTION("Zero size") { + REQUIRE(SimpleLayoutTemplate<>::computeDataSize(0) == 0); + REQUIRE(SimpleLayoutTemplateWithScalar<>::computeDataSize(0) != 0); + } + + SECTION("Default-constructed view") { + SimpleLayout sl; + REQUIRE(sl.metadata().size() == 0); + SimpleLayoutTemplateWithScalar<> sls; + REQUIRE(sls.metadata().size() == 0); + } + const std::size_t slSize = 10; const std::size_t slBufferSize = SimpleLayout::computeDataSize(slSize); std::unique_ptr slBuffer{ From 7f2113b58c02ae0ccf76bc0e321a09c9dab1d80a Mon Sep 17 00:00:00 2001 From: Matti Kortelainen Date: Fri, 26 Apr 2024 20:37:18 +0200 Subject: [PATCH 2/4] Allow DeviceProduct to hold objects that do not have default constructor --- DataFormats/Common/interface/DeviceProduct.h | 10 ++- DataFormats/Common/test/BuildFile.xml | 4 ++ DataFormats/Common/test/DeviceProduct_t.cpp | 70 ++++++++++++++++++++ 3 files changed, 81 insertions(+), 3 deletions(-) create mode 100644 DataFormats/Common/test/DeviceProduct_t.cpp diff --git a/DataFormats/Common/interface/DeviceProduct.h b/DataFormats/Common/interface/DeviceProduct.h index 926d7ad16e262..7af93bff7554f 100644 --- a/DataFormats/Common/interface/DeviceProduct.h +++ b/DataFormats/Common/interface/DeviceProduct.h @@ -3,6 +3,7 @@ #include #include +#include namespace edm { class DeviceProductBase { @@ -49,7 +50,7 @@ namespace edm { template explicit DeviceProduct(std::shared_ptr metadata, Args&&... args) - : DeviceProductBase(std::move(metadata)), data_(std::forward(args)...) {} + : DeviceProductBase(std::move(metadata)), data_(std::in_place_t{}, std::forward(args)...) {} DeviceProduct(const DeviceProduct&) = delete; DeviceProduct& operator=(const DeviceProduct&) = delete; @@ -66,11 +67,14 @@ namespace edm { T const& getSynchronized(Args&&... args) const { auto const& md = metadata(); md.synchronize(std::forward(args)...); - return data_; + return *data_; } private: - T data_; //! + // Using optional to allow T to not have a default constructor + // Default-constructed DeviceProduct should in any case appear + // only when edm::Wrapper::present == false + std::optional data_; //! }; } // namespace edm #endif diff --git a/DataFormats/Common/test/BuildFile.xml b/DataFormats/Common/test/BuildFile.xml index 37e5d74046467..241f4cf908d5c 100644 --- a/DataFormats/Common/test/BuildFile.xml +++ b/DataFormats/Common/test/BuildFile.xml @@ -58,3 +58,7 @@ + + + + diff --git a/DataFormats/Common/test/DeviceProduct_t.cpp b/DataFormats/Common/test/DeviceProduct_t.cpp new file mode 100644 index 0000000000000..05a377274d969 --- /dev/null +++ b/DataFormats/Common/test/DeviceProduct_t.cpp @@ -0,0 +1,70 @@ +#define CATCH_CONFIG_MAIN +#include "catch.hpp" + +#include "DataFormats/Common/interface/DeviceProduct.h" + +namespace { + class Metadata { + public: + Metadata(int v = 0) : value_(v) {} + void synchronize(int& ret) const { ret = value_; } + + private: + int const value_; + }; + + class Product { + public: + Product() = default; + Product(int v) : value_(v) {} + + int value() const { return value_; } + + private: + int value_ = 0; + }; + + class ProductNoDefault { + public: + ProductNoDefault(int v) : value_(v) {} + + int value() const { return value_; } + + private: + int value_; + }; + +} // namespace + +TEST_CASE("DeviceProduct", "[DeviceProduct]") { + SECTION("Default constructor") { [[maybe_unused]] edm::DeviceProduct prod; } + + SECTION("Default-constructible data product") { + SECTION("Default constructed") { + edm::DeviceProduct prod(std::make_shared(3)); + + int md = 0; + auto const& data = prod.getSynchronized(md); + REQUIRE(md == 3); + REQUIRE(data.value() == 0); + } + + SECTION("Explicitly constructed") { + edm::DeviceProduct prod(std::make_shared(4), 42); + + int md = 0; + auto const& data = prod.getSynchronized(md); + REQUIRE(md == 4); + REQUIRE(data.value() == 42); + } + } + + SECTION("Non-default-constructible data product") { + edm::DeviceProduct prod(std::make_shared(5), 314); + + int md = 0; + auto const& data = prod.getSynchronized(md); + REQUIRE(md == 5); + REQUIRE(data.value() == 314); + } +} From b6a555e81d418e6bc6402506d66f1b7caf13341f Mon Sep 17 00:00:00 2001 From: Matti Kortelainen Date: Fri, 26 Apr 2024 22:37:17 +0200 Subject: [PATCH 3/4] Remove default constructor from PortableDeviceCollection --- .../Portable/interface/PortableCollection.h | 7 +- .../interface/PortableDeviceCollection.h | 68 +++++-------------- .../test_catch2_portableCollection.dev.cc | 12 +--- .../interface/SiPixelDigiErrorsDevice.h | 1 - .../interface/SiPixelDigisDevice.h | 1 - .../alpaka/SiPixelPhase2DigiToCluster.cc | 14 ++-- .../plugins/alpaka/SiPixelRawToCluster.cc | 2 +- 7 files changed, 29 insertions(+), 76 deletions(-) diff --git a/DataFormats/Portable/interface/PortableCollection.h b/DataFormats/Portable/interface/PortableCollection.h index 713c7a6084483..2ed4f9642dc78 100644 --- a/DataFormats/Portable/interface/PortableCollection.h +++ b/DataFormats/Portable/interface/PortableCollection.h @@ -50,9 +50,6 @@ namespace cms::alpakatools { struct CopyToHost> { template static auto copyAsync(TQueue& queue, PortableDeviceCollection const& srcData) { - if (not srcData.isValid()) { - return PortableHostCollection(); - } PortableHostCollection dstData(srcData->metadata().size(), queue); alpaka::memcpy(queue, dstData.buffer(), srcData.buffer()); return dstData; @@ -74,9 +71,7 @@ namespace cms::alpakatools { template static auto copyAsync(TQueue& queue, PortableHostCollection const& srcData) { using TDevice = typename alpaka::trait::DevType::type; - if (not srcData.isValid()) { - return PortableDeviceCollection(); - } + assert(srcData.isValid()); PortableDeviceCollection dstData(srcData->metadata().size(), queue); alpaka::memcpy(queue, dstData.buffer(), srcData.buffer()); return dstData; diff --git a/DataFormats/Portable/interface/PortableDeviceCollection.h b/DataFormats/Portable/interface/PortableDeviceCollection.h index ef97122504172..3f9b23455fbf7 100644 --- a/DataFormats/Portable/interface/PortableDeviceCollection.h +++ b/DataFormats/Portable/interface/PortableDeviceCollection.h @@ -24,23 +24,21 @@ class PortableDeviceCollection { using Buffer = cms::alpakatools::device_buffer; using ConstBuffer = cms::alpakatools::const_device_buffer; - PortableDeviceCollection() = default; - PortableDeviceCollection(int32_t elements, TDev const& device) : buffer_{cms::alpakatools::make_device_buffer(device, Layout::computeDataSize(elements))}, - layout_{buffer_->data(), elements}, + layout_{buffer_.data(), elements}, view_{layout_} { // Alpaka set to a default alignment of 128 bytes defining ALPAKA_DEFAULT_HOST_MEMORY_ALIGNMENT=128 - assert(reinterpret_cast(buffer_->data()) % Layout::alignment == 0); + assert(reinterpret_cast(buffer_.data()) % Layout::alignment == 0); } template >> PortableDeviceCollection(int32_t elements, TQueue const& queue) : buffer_{cms::alpakatools::make_device_buffer(queue, Layout::computeDataSize(elements))}, - layout_{buffer_->data(), elements}, + layout_{buffer_.data(), elements}, view_{layout_} { // Alpaka set to a default alignment of 128 bytes defining ALPAKA_DEFAULT_HOST_MEMORY_ALIGNMENT=128 - assert(reinterpret_cast(buffer_->data()) % Layout::alignment == 0); + assert(reinterpret_cast(buffer_.data()) % Layout::alignment == 0); } // non-copyable @@ -55,61 +53,31 @@ class PortableDeviceCollection { ~PortableDeviceCollection() = default; // has a valid buffer? - bool isValid() const { return buffer_.has_value(); } + bool isValid() const { return true; } // a way to access the number of elements without the buffer validity assertion auto size() const { return view_.metadata().size(); } // access the View - View& view() { - assert(isValid()); - return view_; - } - ConstView const& view() const { - assert(isValid()); - return view_; - } - ConstView const& const_view() const { - assert(isValid()); - return view_; - } + View& view() { return view_; } + ConstView const& view() const { return view_; } + ConstView const& const_view() const { return view_; } - View& operator*() { - assert(isValid()); - return view_; - } - ConstView const& operator*() const { - assert(isValid()); - return view_; - } + View& operator*() { return view_; } + ConstView const& operator*() const { return view_; } - View* operator->() { - assert(isValid()); - return &view_; - } - ConstView const* operator->() const { - assert(isValid()); - return &view_; - } + View* operator->() { return &view_; } + ConstView const* operator->() const { return &view_; } // access the Buffer - Buffer buffer() { - assert(isValid()); - return *buffer_; - } - ConstBuffer buffer() const { - assert(isValid()); - return *buffer_; - } - ConstBuffer const_buffer() const { - assert(isValid()); - return *buffer_; - } + Buffer buffer() { return buffer_; } + ConstBuffer buffer() const { return buffer_; } + ConstBuffer const_buffer() const { return buffer_; } private: - std::optional buffer_; //! - Layout layout_; // - View view_; //! + Buffer buffer_; //! + Layout layout_; // + View view_; //! }; // generic SoA-based product in device memory diff --git a/DataFormats/Portable/test/alpaka/test_catch2_portableCollection.dev.cc b/DataFormats/Portable/test/alpaka/test_catch2_portableCollection.dev.cc index 0a6977b3d8af4..d8ca8199fba37 100644 --- a/DataFormats/Portable/test/alpaka/test_catch2_portableCollection.dev.cc +++ b/DataFormats/Portable/test/alpaka/test_catch2_portableCollection.dev.cc @@ -36,16 +36,8 @@ TEST_CASE("PortableCollection", s_tag) { //coll->num() = 42; //REQUIRE(coll->num() == 42); - // CopyToDevice> is not defined -#ifndef ALPAKA_ACC_CPU_B_SEQ_T_SEQ_ENABLED - for (auto const& device : devices) { - auto queue = Queue(device); - auto coll_d = cms::alpakatools::CopyToDevice>::copyAsync(queue, coll_h); - REQUIRE(coll_d.size() == 0); - REQUIRE(not coll_d.isValid()); - alpaka::wait(queue); - } -#endif + // This would lead to an assertion failure + // cms::alpakatools::CopyToDevice>::copyAsync(queue, coll_h) } SECTION("Zero size") { diff --git a/DataFormats/SiPixelDigiSoA/interface/SiPixelDigiErrorsDevice.h b/DataFormats/SiPixelDigiSoA/interface/SiPixelDigiErrorsDevice.h index 36c7d0be7e88a..e12c869aa2d4d 100644 --- a/DataFormats/SiPixelDigiSoA/interface/SiPixelDigiErrorsDevice.h +++ b/DataFormats/SiPixelDigiSoA/interface/SiPixelDigiErrorsDevice.h @@ -14,7 +14,6 @@ template class SiPixelDigiErrorsDevice : public PortableDeviceCollection { public: - SiPixelDigiErrorsDevice() = default; template explicit SiPixelDigiErrorsDevice(size_t maxFedWords, TQueue queue) : PortableDeviceCollection(maxFedWords, queue), maxFedWords_(maxFedWords) {} diff --git a/DataFormats/SiPixelDigiSoA/interface/SiPixelDigisDevice.h b/DataFormats/SiPixelDigiSoA/interface/SiPixelDigisDevice.h index 1748069685923..0eef837629951 100644 --- a/DataFormats/SiPixelDigiSoA/interface/SiPixelDigisDevice.h +++ b/DataFormats/SiPixelDigiSoA/interface/SiPixelDigisDevice.h @@ -12,7 +12,6 @@ template class SiPixelDigisDevice : public PortableDeviceCollection { public: - SiPixelDigisDevice() = default; template explicit SiPixelDigisDevice(size_t maxFedWords, TQueue queue) : PortableDeviceCollection(maxFedWords + 1, queue) {} diff --git a/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelPhase2DigiToCluster.cc b/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelPhase2DigiToCluster.cc index 575c5ab925145..416c8fd614d1a 100644 --- a/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelPhase2DigiToCluster.cc +++ b/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelPhase2DigiToCluster.cc @@ -54,7 +54,7 @@ namespace ALPAKA_ACCELERATOR_NAMESPACE { const SiPixelClusterThresholds clusterThresholds_; uint32_t nDigis_ = 0; - SiPixelDigisSoACollection digis_d; + std::optional digis_d; }; SiPixelPhase2DigiToCluster::SiPixelPhase2DigiToCluster(const edm::ParameterSet& iConfig) @@ -128,25 +128,25 @@ namespace ALPAKA_ACCELERATOR_NAMESPACE { } digis_d = SiPixelDigisSoACollection(nDigis, iEvent.queue()); - alpaka::memcpy(iEvent.queue(), digis_d.buffer(), digis_h.buffer()); + alpaka::memcpy(iEvent.queue(), digis_d->buffer(), digis_h.buffer()); - Algo_.makePhase2ClustersAsync(iEvent.queue(), clusterThresholds_, digis_d.view(), nDigis); + Algo_.makePhase2ClustersAsync(iEvent.queue(), clusterThresholds_, digis_d->view(), nDigis); } void SiPixelPhase2DigiToCluster::produce(device::Event& iEvent, device::EventSetup const& iSetup) { if (nDigis_ == 0) { SiPixelClustersSoACollection clusters_d{pixelTopology::Phase1::numberOfModules, iEvent.queue()}; - iEvent.emplace(digiPutToken_, std::move(digis_d)); + iEvent.emplace(digiPutToken_, std::move(*digis_d)); iEvent.emplace(clusterPutToken_, std::move(clusters_d)); if (includeErrors_) { - iEvent.emplace(digiErrorPutToken_, SiPixelDigiErrorsSoACollection()); + iEvent.emplace(digiErrorPutToken_, SiPixelDigiErrorsSoACollection(0, iEvent.queue())); } return; } - digis_d.setNModulesDigis(Algo_.nModules(), nDigis_); + digis_d->setNModulesDigis(Algo_.nModules(), nDigis_); - iEvent.emplace(digiPutToken_, std::move(digis_d)); + iEvent.emplace(digiPutToken_, std::move(*digis_d)); iEvent.emplace(clusterPutToken_, Algo_.getClusters()); if (includeErrors_) { iEvent.emplace(digiErrorPutToken_, Algo_.getErrors()); diff --git a/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelRawToCluster.cc b/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelRawToCluster.cc index 8a74cc67767d3..cb6c5f64e46c9 100644 --- a/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelRawToCluster.cc +++ b/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelRawToCluster.cc @@ -272,7 +272,7 @@ namespace ALPAKA_ACCELERATOR_NAMESPACE { iEvent.emplace(digiPutToken_, nDigis_, iEvent.queue()); iEvent.emplace(clusterPutToken_, pixelTopology::Phase1::numberOfModules, iEvent.queue()); if (includeErrors_) { - iEvent.emplace(digiErrorPutToken_); + iEvent.emplace(digiErrorPutToken_, 0, iEvent.queue()); iEvent.emplace(fmtErrorToken_); } return; From e4d2b5343f939259247ead48d68c9c2754724d23 Mon Sep 17 00:00:00 2001 From: Matti Kortelainen Date: Mon, 29 Apr 2024 22:13:47 +0200 Subject: [PATCH 4/4] Make moved-from state of PortableHostCollection well defined --- .../interface/PortableHostCollection.h | 30 +++++++++++++++-- .../test_catch2_portableHostCollection.cc | 33 +++++++++++++++++++ 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/DataFormats/Portable/interface/PortableHostCollection.h b/DataFormats/Portable/interface/PortableHostCollection.h index f21deb45bba4e..cff6e94bd08ac 100644 --- a/DataFormats/Portable/interface/PortableHostCollection.h +++ b/DataFormats/Portable/interface/PortableHostCollection.h @@ -12,6 +12,9 @@ #include "DataFormats/Portable/interface/PortableCollectionCommon.h" // generic SoA-based product in host memory +// +// Moving from PortableHostCollection leaves the source object in a +// "not isValid()" state that has 0 size template class PortableHostCollection { public: @@ -47,8 +50,31 @@ class PortableHostCollection { PortableHostCollection& operator=(PortableHostCollection const&) = delete; // movable - PortableHostCollection(PortableHostCollection&&) = default; - PortableHostCollection& operator=(PortableHostCollection&&) = default; + PortableHostCollection(PortableHostCollection&& other): + buffer_(std::move(other.buffer_)), + layout_(std::move(other.layout_)), + view_(std::move(other.view_)) + { + other.buffer_.reset(); + other.view_ = View(); + } + PortableHostCollection& operator=(PortableHostCollection&& other) { + // Protection for self-assignment without if-statements, following + // what was suggested in the C++ core guidelines + { + auto tmp = std::move(other.buffer_); + other.buffer_.reset(); + buffer_.reset(); + buffer_ = std::move(tmp); + } + { + auto tmp = std::move(other.view_); + other.view_ = View(); + view_ = View(); + view_ = std::move(tmp); + } + return *this; + } // default destructor ~PortableHostCollection() = default; diff --git a/DataFormats/Portable/test/test_catch2_portableHostCollection.cc b/DataFormats/Portable/test/test_catch2_portableHostCollection.cc index 6eb6e640b3ea1..6fc41cb2cca8b 100644 --- a/DataFormats/Portable/test/test_catch2_portableHostCollection.cc +++ b/DataFormats/Portable/test/test_catch2_portableHostCollection.cc @@ -49,5 +49,38 @@ TEST_CASE("PortableHostCollection", s_tag) { for (int i = 0; i < size; ++i) { REQUIRE(coll->id()[i] == i * 2); } + + SECTION("Move constructor") { + PortableHostCollection coll2(std::move(coll)); + REQUIRE(coll2.size() == size); + REQUIRE(coll2.isValid()); + + REQUIRE(coll.size() == 0); + REQUIRE(not coll.isValid()); + } + + SECTION("Move assignment") { + PortableHostCollection coll2(2*size, cms::alpakatools::host()); + REQUIRE(coll2.size() == 2*size); + REQUIRE(coll2.isValid()); + + coll2 = std::move(coll); + REQUIRE(coll2.size() == size); + REQUIRE(coll2.isValid()); + + REQUIRE(coll.size() == 0); + REQUIRE(not coll.isValid()); + + SECTION("Self assignment") { + coll2 = std::move(coll2); + REQUIRE(coll2.size() == size); + REQUIRE(coll2.isValid()); + + REQUIRE(coll2->num() == 42); + for (int i = 0; i < size; ++i) { + REQUIRE(coll2->id()[i] == i * 2); + } + } + } } }