diff --git a/include/buffer_manager.h b/include/buffer_manager.h index e5969caff..a6ea637eb 100644 --- a/include/buffer_manager.h +++ b/include/buffer_manager.h @@ -278,9 +278,13 @@ namespace detail { m_buffer_infos.at(bid).debug_name = debug_name; } - std::string get_debug_name(const buffer_id bid) const { - std::lock_guard lock(m_mutex); - return m_buffer_infos.at(bid).debug_name; + std::string get_debug_label(const buffer_id bid) const { + std::string name; + { + std::lock_guard lock(m_mutex); + name = m_buffer_infos.at(bid).debug_name; + } + return !name.empty() ? fmt::format("B{} \"{}\"", bid, name) : fmt::format("B{}", bid); } private: diff --git a/include/debug.h b/include/debug.h index 0b63c87c0..929bf6a81 100644 --- a/include/debug.h +++ b/include/debug.h @@ -15,8 +15,6 @@ namespace debug { return detail::get_buffer_name(buff); } - inline void set_task_name(celerity::handler& cgh, const std::string& debug_name) { - detail::set_task_name(cgh, debug_name); - } + inline void set_task_name(celerity::handler& cgh, const std::string& debug_name) { detail::set_task_name(cgh, debug_name); } } // namespace debug } // namespace celerity \ No newline at end of file diff --git a/include/handler.h b/include/handler.h index a153e3fb6..d2afad3a9 100644 --- a/include/handler.h +++ b/include/handler.h @@ -376,7 +376,7 @@ class handler { friend void detail::add_reduction(handler& cgh, const detail::reduction_info& rinfo); friend void detail::extend_lifetime(handler& cgh, std::shared_ptr state); - friend void detail::set_task_name(handler &cgh, const std::string& debug_name); + friend void detail::set_task_name(handler& cgh, const std::string& debug_name); detail::task_id m_tid; detail::buffer_access_map m_access_map; @@ -462,11 +462,9 @@ class handler { } // Note that cgf_diagnostics has a similar check, but we don't catch void side effects there. if(!m_side_effects.empty()) { throw std::runtime_error{"Side effects cannot be used in device kernels"}; } - m_task = - detail::task::make_device_compute(m_tid, geometry, std::move(launcher), std::move(m_access_map), std::move(m_reductions)); + m_task = detail::task::make_device_compute(m_tid, geometry, std::move(launcher), std::move(m_access_map), std::move(m_reductions)); m_task->set_debug_name(m_usr_def_task_name.value_or(debug_name)); - } void create_collective_task(detail::collective_group_id cgid, std::unique_ptr launcher) { @@ -588,9 +586,7 @@ namespace detail { inline void extend_lifetime(handler& cgh, std::shared_ptr state) { cgh.extend_lifetime(std::move(state)); } - inline void set_task_name(handler& cgh, const std::string& debug_name) { - cgh.m_usr_def_task_name = {debug_name}; - } + inline void set_task_name(handler& cgh, const std::string& debug_name) { cgh.m_usr_def_task_name = {debug_name}; } // TODO: The _impl functions in detail only exist during the grace period for deprecated reductions on const buffers; move outside again afterwards. template diff --git a/src/print_graph.cc b/src/print_graph.cc index 0bba72cb4..32e677c59 100644 --- a/src/print_graph.cc +++ b/src/print_graph.cc @@ -34,25 +34,17 @@ const char* task_type_string(const task_type tt) { } } -std::string get_buffer_label(const buffer_id bid, const std::string& name = "") { - // if there is no name defined, the name will be the buffer id. - // if there is a name we want "id name" - return !name.empty() ? fmt::format("B{} \"{}\"", bid, name) : fmt::format("B{}", bid); -} - void format_requirements(std::string& label, const reduction_list& reductions, const access_list& accesses, const side_effect_map& side_effects, const access_mode reduction_init_mode) { for(const auto& [rid, bid, buffer_name, init_from_buffer] : reductions) { auto rmode = init_from_buffer ? reduction_init_mode : cl::sycl::access::mode::discard_write; const region scalar_region(box<3>({0, 0, 0}, {1, 1, 1})); - const std::string bl = get_buffer_label(bid, buffer_name); - fmt::format_to(std::back_inserter(label), "
(R{}) {} {} {}", rid, detail::access::mode_traits::name(rmode), bl, scalar_region); + fmt::format_to(std::back_inserter(label), "
(R{}) {} {} {}", rid, detail::access::mode_traits::name(rmode), buffer_name, scalar_region); } for(const auto& [bid, buffer_name, mode, req] : accesses) { - const std::string bl = get_buffer_label(bid, buffer_name); // While uncommon, we do support chunks that don't require access to a particular buffer at all. - if(!req.empty()) { fmt::format_to(std::back_inserter(label), "
{} {} {}", detail::access::mode_traits::name(mode), bl, req); } + if(!req.empty()) { fmt::format_to(std::back_inserter(label), "
{} {} {}", detail::access::mode_traits::name(mode), buffer_name, req); } } for(const auto& [hoid, order] : side_effects) { @@ -102,7 +94,7 @@ std::string get_command_label(const node_id local_nid, const command_record& cmd auto add_reduction_id_if_reduction = [&]() { if(cmd.reduction_id.has_value() && cmd.reduction_id != 0) { fmt::format_to(std::back_inserter(label), "(R{}) ", cmd.reduction_id.value()); } }; - const std::string buffer_label = cmd.buffer_id.has_value() ? get_buffer_label(cmd.buffer_id.value(), cmd.buffer_name) : ""; + const std::string buffer_label = cmd.buffer_id.has_value() ? cmd.buffer_name : ""; switch(cmd.type) { case command_type::epoch: { diff --git a/src/recorders.cc b/src/recorders.cc index 1b7fae260..dde5cb740 100644 --- a/src/recorders.cc +++ b/src/recorders.cc @@ -9,7 +9,7 @@ namespace celerity::detail { // Naming std::string get_buffer_name(const buffer_id bid, const buffer_manager* buff_man) { - return buff_man != nullptr ? buff_man->get_buffer_info(bid).debug_name : ""; + return buff_man != nullptr ? buff_man->get_debug_label(bid) : fmt::format("B{}", bid); } // Tasks @@ -91,7 +91,7 @@ std::optional get_buffer_id(const abstract_command& cmd) { } std::string get_cmd_buffer_name(const std::optional& bid, const buffer_manager* buff_mngr) { - if(buff_mngr == nullptr || !bid.has_value()) return ""; + if(!bid.has_value()) return ""; return get_buffer_name(bid.value(), buff_mngr); } diff --git a/src/worker_job.cc b/src/worker_job.cc index 5a1351649..c8e71370e 100644 --- a/src/worker_job.cc +++ b/src/worker_job.cc @@ -202,10 +202,9 @@ namespace detail { const auto acc_sr = access_map.get_requirements_for_nth_access(i, tsk->get_dimensions(), data.sr, tsk->get_global_size()).get_subrange(); const auto oob_sr = subrange<3>(oob_min, range_cast<3>(oob_max - oob_min)); const auto buffer_id = access_map.get_nth_access(i).first; - const auto buffer_name = m_buffer_mngr.get_debug_name(buffer_id); CELERITY_ERROR("Out-of-bounds access in host task detected: Accessor {} for buffer {} attempted to access indices between {} which are " "outside of mapped subrange {}", - i, (buffer_name.empty() ? fmt::format("{}", buffer_id) : buffer_name), oob_sr, acc_sr); + i, m_buffer_mngr.get_debug_label(buffer_id), oob_sr, acc_sr); } } #endif @@ -301,10 +300,9 @@ namespace detail { const auto acc_sr = access_map.get_requirements_for_nth_access(i, tsk->get_dimensions(), data.sr, tsk->get_global_size()).get_subrange(); const auto oob_sr = subrange<3>(oob_min, range_cast<3>(oob_max - oob_min)); const auto buffer_id = access_map.get_nth_access(i).first; - const auto buffer_name = m_buffer_mngr.get_debug_name(buffer_id); CELERITY_ERROR("Out-of-bounds access in kernel '{}' detected: Accessor {} for buffer {} attempted to access indices between {} which are " "outside of mapped subrange {}", - tsk->get_debug_name(), i, (buffer_name.empty() ? fmt::format("{}", buffer_id) : buffer_name), oob_sr, acc_sr); + tsk->get_debug_name(), i, m_buffer_mngr.get_debug_label(buffer_id), oob_sr, acc_sr); } sycl::free(m_oob_indices_per_accessor[i], m_queue.get_sycl_queue()); } diff --git a/test/accessor_tests.cc b/test/accessor_tests.cc index e49552e7b..3143dfaf9 100644 --- a/test/accessor_tests.cc +++ b/test/accessor_tests.cc @@ -689,15 +689,15 @@ namespace detail { const auto attempted_sr = subrange<3>{id_cast<3>(oob_idx_lo), range_cast<3>(oob_idx_hi - oob_idx_lo + id_cast(range(ones))) - range_cast<3>(range(zeros))}; const auto unnamed_error_message = - fmt::format("Out-of-bounds access in kernel 'celerity::detail::acc_out_of_bounds_kernel<{}>' detected: Accessor 0 for buffer 0 attempted to " + fmt::format("Out-of-bounds access in kernel 'celerity::detail::acc_out_of_bounds_kernel<{}>' detected: Accessor 0 for buffer B0 attempted to " "access indices between {} which are outside of mapped subrange {}", Dims, attempted_sr, subrange_cast<3>(accessible_sr)); CHECK_THAT(lc->get_log(), Catch::Matchers::ContainsSubstring(unnamed_error_message)); - const auto named_error_message = - fmt::format("Out-of-bounds access in kernel 'celerity::detail::acc_out_of_bounds_kernel<{}>' detected: Accessor 1 for buffer {} attempted to " - "access indices between {} which are outside of mapped subrange {}", - Dims, buffer_name, attempted_sr, subrange_cast<3>(accessible_sr)); + const auto named_error_message = fmt::format( + "Out-of-bounds access in kernel 'celerity::detail::acc_out_of_bounds_kernel<{}>' detected: Accessor 1 for buffer B1 \"{}\" attempted to " + "access indices between {} which are outside of mapped subrange {}", + Dims, buffer_name, attempted_sr, subrange_cast<3>(accessible_sr)); CHECK_THAT(lc->get_log(), Catch::Matchers::ContainsSubstring(named_error_message)); } @@ -740,12 +740,12 @@ namespace detail { const auto attempted_sr = subrange<3>{id_cast<3>(oob_idx_lo), range_cast<3>(oob_idx_hi - oob_idx_lo + id_cast(range(ones))) - range_cast<3>(range(zeros))}; - const auto unnamed_error_message = fmt::format("Out-of-bounds access in host task detected: Accessor 0 for buffer 0 attempted to " + const auto unnamed_error_message = fmt::format("Out-of-bounds access in host task detected: Accessor 0 for buffer B0 attempted to " "access indices between {} which are outside of mapped subrange {}", attempted_sr, subrange_cast<3>(accessible_sr)); CHECK_THAT(lc->get_log(), Catch::Matchers::ContainsSubstring(unnamed_error_message)); - const auto named_error_message = fmt::format("Out-of-bounds access in host task detected: Accessor 1 for buffer {} attempted to " + const auto named_error_message = fmt::format("Out-of-bounds access in host task detected: Accessor 1 for buffer B1 \"{}\" attempted to " "access indices between {} which are outside of mapped subrange {}", buffer_name, attempted_sr, subrange_cast<3>(accessible_sr)); CHECK_THAT(lc->get_log(), Catch::Matchers::ContainsSubstring(named_error_message)); diff --git a/test/debug_naming_tests.cc b/test/debug_naming_tests.cc index 803916b45..09b7385b8 100644 --- a/test/debug_naming_tests.cc +++ b/test/debug_naming_tests.cc @@ -19,9 +19,7 @@ TEST_CASE("debug names can be set and retrieved from tasks", "[debug]") { auto tt = test_utils::task_test_context{}; SECTION("Host Task") { - const auto tid_a = test_utils::add_host_task(tt.tm, on_master_node, [&](handler& cgh) { - celerity::debug::set_task_name(cgh, task_name); - }); + const auto tid_a = test_utils::add_host_task(tt.tm, on_master_node, [&](handler& cgh) { celerity::debug::set_task_name(cgh, task_name); }); const auto tid_b = test_utils::add_host_task(tt.tm, on_master_node, [&](handler& cgh) {}); @@ -30,9 +28,7 @@ TEST_CASE("debug names can be set and retrieved from tasks", "[debug]") { } SECTION("Compute Task") { - const auto tid_a = test_utils::add_compute_task(tt.tm, [&](handler& cgh) { - celerity::debug::set_task_name(cgh, task_name); - }); + const auto tid_a = test_utils::add_compute_task(tt.tm, [&](handler& cgh) { celerity::debug::set_task_name(cgh, task_name); }); const auto tid_b = test_utils::add_compute_task(tt.tm, [&](handler& cgh) {}); @@ -41,9 +37,8 @@ TEST_CASE("debug names can be set and retrieved from tasks", "[debug]") { } SECTION("ND Range Task") { - const auto tid_a = test_utils::add_nd_range_compute_task(tt.tm, [&](handler& cgh) { - celerity::debug::set_task_name(cgh, task_name); - }); + const auto tid_a = + test_utils::add_nd_range_compute_task(tt.tm, [&](handler& cgh) { celerity::debug::set_task_name(cgh, task_name); }); const auto tid_b = test_utils::add_compute_task(tt.tm, [&](handler& cgh) {}); @@ -55,13 +50,14 @@ TEST_CASE("debug names can be set and retrieved from tasks", "[debug]") { TEST_CASE_METHOD(test_utils::runtime_fixture, "buffer_manager allows to set buffer debug names on buffers", "[buffer_manager]") { celerity::buffer buff_a(16); std::string buff_name{"my_buffer"}; + std::string buffer_label{fmt::format("B{} \"{}\"", detail::get_buffer_id(buff_a), buff_name)}; celerity::detail::runtime::get_instance().get_buffer_manager().set_debug_name(celerity::detail::get_buffer_id(buff_a), buff_name); - CHECK(celerity::detail::runtime::get_instance().get_buffer_manager().get_debug_name(celerity::detail::get_buffer_id(buff_a)) == buff_name); + CHECK(detail::runtime::get_instance().get_buffer_manager().get_debug_label(detail::get_buffer_id(buff_a)) == buffer_label); } namespace foo { - class MySecondKernel; +class MySecondKernel; } template