From 7a5326a4c5edf348deedd6f9a7f6e9360ea22684 Mon Sep 17 00:00:00 2001 From: Fabian Knorr Date: Tue, 1 Feb 2022 10:11:02 +0100 Subject: [PATCH] Side effects: address reviewer comments on code style --- include/host_object.h | 24 +++++++++++++++-------- include/side_effect.h | 8 ++++++-- test/graph_generation_tests.cc | 36 +++++++++++++++++++++------------- 3 files changed, 44 insertions(+), 24 deletions(-) diff --git a/include/host_object.h b/include/host_object.h index b24600051..0e5669367 100644 --- a/include/host_object.h +++ b/include/host_object.h @@ -1,5 +1,11 @@ #pragma once +#include +#include +#include +#include +#include + #include "runtime.h" @@ -68,14 +74,16 @@ using assert_host_object_ctor_param_is_rvalue_t = typename assert_host_object_ct namespace celerity::experimental { -/// A `host_object` wraps state that exists separately on each worker node and can be referenced in host tasks through `side_effect`s. Celerity ensures that -/// access to the object state is properly synchronized and ordered. An example usage of a host object might be a file stream that is written to from multiple -/// host tasks sequentially. -/// -/// - The generic `host_object` keeps ownership of the state at any time and is the safest way to achieve side effects on the host. -/// - The `host_object` specialization attaches Celerity's tracking and synchronization mechanism to user-managed state. The user guarantees that the -/// referenced object is not accessed in any way other than through a `side_effect` while the `host_object` is live. -/// - `host_object` does not carry internal state and can be used to track access to global variables or functions like `printf()`. +/** + * A `host_object` wraps state that exists separately on each worker node and can be referenced in host tasks through `side_effect`s. Celerity ensures that + * access to the object state is properly synchronized and ordered. An example usage of a host object might be a file stream that is written to from multiple + * host tasks sequentially. + * + * - The generic `host_object` keeps ownership of the state at any time and is the safest way to achieve side effects on the host. + * - The `host_object` specialization attaches Celerity's tracking and synchronization mechanism to user-managed state. The user guarantees that the + * referenced object is not accessed in any way other than through a `side_effect` while the `host_object` is live. + * - `host_object` does not carry internal state and can be used to track access to global variables or functions like `printf()`. + */ template class host_object { static_assert(std::is_object_v); // disallow host_object and host_object diff --git a/include/side_effect.h b/include/side_effect.h index 43a43d9c8..24f35bc0c 100644 --- a/include/side_effect.h +++ b/include/side_effect.h @@ -1,13 +1,17 @@ #pragma once +#include + #include "handler.h" #include "host_object.h" namespace celerity::experimental { -/// Provides access to a `host_object` through capture in a `host_task`. Inside the host task kernel, the internal state of the host object can be accessed -/// through the `*` or `->` operators. This behavior is similar to accessors on buffers. +/** + * Provides access to a `host_object` through capture in a `host_task`. Inside the host task kernel, the internal state of the host object can be accessed + * through the `*` or `->` operators. This behavior is similar to accessors on buffers. + */ template class side_effect { public: diff --git a/test/graph_generation_tests.cc b/test/graph_generation_tests.cc index 518b0fd33..15f2d7617 100644 --- a/test/graph_generation_tests.cc +++ b/test/graph_generation_tests.cc @@ -1525,8 +1525,12 @@ namespace detail { TEST_CASE("side effects generate appropriate command-dependencies", "[graph_generator][command-graph][side-effect]") { using order = experimental::side_effect_order; + + // Must be static for Catch2 GENERATE, which implicitly generates sections for each value and therefore cannot depend on runtime values static constexpr auto side_effect_orders = {order::sequential}; + constexpr size_t num_nodes = 2; + const range<1> node_range{num_nodes}; // TODO placeholder: complete with dependency types for other side effect orders const auto expected_dependencies = std::unordered_map, std::optional, pair_hash>{ @@ -1546,31 +1550,35 @@ namespace detail { auto ho_common = mhof.create_host_object(); // should generate dependencies auto ho_a = mhof.create_host_object(); // should NOT generate dependencies auto ho_b = mhof.create_host_object(); // -"- - const auto tid_a = test_utils::build_and_flush(ctx, num_nodes, test_utils::add_host_task(tm, sycl::range<1>{num_nodes}, [&](handler& cgh) { - ho_common.add_side_effect(cgh, order_a); + const auto tid_0 = test_utils::build_and_flush(ctx, num_nodes, test_utils::add_host_task(tm, node_range, [&](handler& cgh) { // ho_a.add_side_effect(cgh, order_a); })); - const auto tid_b = test_utils::build_and_flush(ctx, num_nodes, test_utils::add_host_task(tm, sycl::range<1>{num_nodes}, [&](handler& cgh) { - ho_common.add_side_effect(cgh, order_b); + const auto tid_1 = test_utils::build_and_flush(ctx, num_nodes, test_utils::add_host_task(tm, node_range, [&](handler& cgh) { // + ho_common.add_side_effect(cgh, order_a); ho_b.add_side_effect(cgh, order_b); })); + const auto tid_2 = test_utils::build_and_flush(ctx, num_nodes, test_utils::add_host_task(tm, node_range, [&](handler& cgh) { // + ho_common.add_side_effect(cgh, order_b); + })); auto& inspector = ctx.get_inspector(); auto& cdag = ctx.get_command_graph(); - for(auto cid_a : inspector.get_commands(tid_a, std::nullopt, std::nullopt)) { - const auto deps_a = cdag.get(cid_a)->get_dependencies(); - CHECK(deps_a.empty()); + for(auto tid : {tid_0, tid_1}) { + for(auto cid : inspector.get_commands(tid, std::nullopt, std::nullopt)) { + const auto deps = cdag.get(cid)->get_dependencies(); + CHECK(deps.empty()); + } } - const auto expected_b = expected_dependencies.at({order_a, order_b}); - for(auto cid_b : inspector.get_commands(tid_b, std::nullopt, std::nullopt)) { - const auto deps_b = cdag.get(cid_b)->get_dependencies(); + const auto expected_2 = expected_dependencies.at({order_a, order_b}); + for(auto cid_2 : inspector.get_commands(tid_2, std::nullopt, std::nullopt)) { + const auto deps_2 = cdag.get(cid_2)->get_dependencies(); // This assumes no oversubscription in the split, adjust if necessary: - CHECK(std::distance(deps_b.begin(), deps_b.end()) == expected_b.has_value()); - if(expected_b) { - const auto& dep_tcmd = dynamic_cast(*deps_b.front().node); - CHECK(dep_tcmd.get_tid() == tid_a); + CHECK(std::distance(deps_2.begin(), deps_2.end()) == expected_2.has_value()); + if(expected_2) { + const auto& dep_tcmd = dynamic_cast(*deps_2.front().node); + CHECK(dep_tcmd.get_tid() == tid_1); } } }