Add tests and rudimentary protections for default-constructed Portabl…

…eCollections Also add tests for zero-sized PortableCollections
cms-sw · Apr 24, 2024 · c30702c · c30702c
1 parent 427fc3b
commit c30702c
Show file tree

Hide file tree

Showing 8 changed files with 299 additions and 20 deletions.
diff --git a/DataFormats/Portable/interface/PortableCollection.h b/DataFormats/Portable/interface/PortableCollection.h
@@ -50,6 +50,9 @@ namespace cms::alpakatools {
   struct CopyToHost<PortableDeviceCollection<TLayout, TDevice>> {
     template <typename TQueue>
     static auto copyAsync(TQueue& queue, PortableDeviceCollection<TLayout, TDevice> const& srcData) {
+      if (not srcData.isValid()) {
+        return PortableHostCollection<TLayout>();
+      }
       PortableHostCollection<TLayout> dstData(srcData->metadata().size(), queue);
       alpaka::memcpy(queue, dstData.buffer(), srcData.buffer());
       return dstData;
@@ -71,6 +74,9 @@ namespace cms::alpakatools {
     template <typename TQueue>
     static auto copyAsync(TQueue& queue, PortableHostCollection<TLayout> const& srcData) {
       using TDevice = typename alpaka::trait::DevType<TQueue>::type;
+      if (not srcData.isValid()) {
+        return PortableDeviceCollection<TLayout, TDevice>();
+      }
       PortableDeviceCollection<TLayout, TDevice> 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
@@ -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> buffer_;  //!

diff --git 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
@@ -3,3 +3,12 @@
   <use name="DataFormats/SoATemplate"/>
   <use name="catch2"/>
 </bin>
+
+<bin name="TestDataFormatsPortableCollection" file="alpaka/test_catch2_*.cc">
+  <use name="DataFormats/Portable"/>
+  <use name="DataFormats/SoATemplate"/>
+  <use name="HeterogeneousCore/AlpakaInterface"/>
+  <use name="alpaka"/>
+  <use name="catch2"/>
+  <flags ALPAKA_BACKENDS="1"/>
+</bin>
diff --git a/DataFormats/Portable/test/alpaka/test_catch2_main.cc b/DataFormats/Portable/test/alpaka/test_catch2_main.cc
@@ -0,0 +1,2 @@
+#define CATCH_CONFIG_MAIN
+#include <catch.hpp>
diff --git a/DataFormats/Portable/test/alpaka/test_catch2_portableCollection.dev.cc b/DataFormats/Portable/test/alpaka/test_catch2_portableCollection.dev.cc
@@ -0,0 +1,121 @@
+#include <catch.hpp>
+
+#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<T, TDev>", s_tag) {
+  // get the list of devices on the current platform
+  auto const& devices = cms::alpakatools::devices<Platform>();
+  if (devices.empty()) {
+    FAIL("No devices available for the " EDM_STRINGIZE(ALPAKA_ACCELERATOR_NAMESPACE) " backend, "
+         "the test will be skipped.");
+  }
+
+  SECTION("Default constructor") {
+    PortableHostCollection<TestSoA> 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<PortableHostCollection<T>> 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<PortableHostCollection<TestSoA>>::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<TestSoA> coll_h(size, cms::alpakatools::host());
+    REQUIRE(coll_h.isValid());
+    REQUIRE(coll_h->metadata().size() == size);
+    coll_h->num() = 42;
+
+    // CopyToDevice<PortableHostCollection<T>> 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<PortableHostCollection<TestSoA>>::copyAsync(queue, coll_h);
+      REQUIRE(coll_d.isValid());
+      REQUIRE(coll_d.size() == size);
+
+      auto div = cms::alpakatools::make_workdiv<Acc1D>(1, 1);
+      alpaka::exec<Acc1D>(
+          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<TestSoA> 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<PortableHostCollection<T>> 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<PortableHostCollection<TestSoA>>::copyAsync(queue, coll_h);
+      REQUIRE(coll_d.isValid());
+      REQUIRE(coll_d.size() == size);
+
+      auto div = cms::alpakatools::make_workdiv<Acc1D>(1, size);
+      alpaka::exec<Acc1D>(
+          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
@@ -0,0 +1,53 @@
+#include <catch.hpp>
+
+#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<T>", s_tag) {
+  SECTION("Default constructor") {
+    PortableHostCollection<TestSoA> 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<TestSoA> 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<TestSoA> 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
@@ -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<std::byte, decltype(std::free) *> slBuffer{
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		#define CATCH_CONFIG_MAIN
		#include <catch.hpp>