From effcdcb4089d17462a683ccecb7ee6ae3abf4600 Mon Sep 17 00:00:00 2001 From: Mikael Simberg Date: Wed, 6 Nov 2024 17:05:19 +0100 Subject: [PATCH 01/14] Test lifetimes of transform_mpi parameters --- .../async_mpi/tests/unit/algorithm_transform_mpi.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/libs/pika/async_mpi/tests/unit/algorithm_transform_mpi.cpp b/libs/pika/async_mpi/tests/unit/algorithm_transform_mpi.cpp index c4093ede9..3b030dbaf 100644 --- a/libs/pika/async_mpi/tests/unit/algorithm_transform_mpi.cpp +++ b/libs/pika/async_mpi/tests/unit/algorithm_transform_mpi.cpp @@ -200,6 +200,18 @@ int pika_main() PIKA_TEST_EQ(data, 42); } + // Values passed to transform_mpi should be kept alive by transform_mpi itself + { + int count = 1 << 20; + auto s = ex::just(std::vector{count, 0}, datatype, 0, comm) | + ex::drop_operation_state() | + mpi::transform_mpi([](auto& data, MPI_Datatype datatype, int i, MPI_Comm comm, + MPI_Request* request) { + MPI_Ibcast(data.data(), data.size(), datatype, i, comm, request); + }); + tt::sync_wait(PIKA_MOVE(s)); + } + // transform_mpi should be able to handle reference types (by copying // them to the operation state) { From 4bebf8377f409fd5fd571adaf0469d833cdd31a0 Mon Sep 17 00:00:00 2001 From: John Biddiscombe Date: Fri, 8 Nov 2024 17:08:40 +0100 Subject: [PATCH 02/14] fix __has_feature undefined problem when using gcc --- .../config/include/pika/config/compiler_specific.hpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/libs/pika/config/include/pika/config/compiler_specific.hpp b/libs/pika/config/include/pika/config/compiler_specific.hpp index 1526da789..2b851fa98 100644 --- a/libs/pika/config/include/pika/config/compiler_specific.hpp +++ b/libs/pika/config/include/pika/config/compiler_specific.hpp @@ -162,14 +162,20 @@ #endif // clang-format on +# if !defined(__has_feature) +# define PIKA_HAS_FEATURE(x) 0 +# else +# define PIKA_HAS_FEATURE(x) __has_feature(x) +# endif + # if defined(PIKA_HAVE_SANITIZERS) -# if defined(__SANITIZE_ADDRESS__) || (defined(__has_feature) && __has_feature(address_sanitizer)) +# if defined(__SANITIZE_ADDRESS__) || PIKA_HAS_FEATURE(address_sanitizer) # define PIKA_HAVE_ADDRESS_SANITIZER # if defined(PIKA_GCC_VERSION) || defined(PIKA_CLANG_VERSION) # define PIKA_NO_SANITIZE_ADDRESS __attribute__((no_sanitize("address"))) # endif # endif -# if defined(__SANITIZE_THREAD__) || (defined(__has_feature) && __has_feature(thread_sanitizer)) +# if defined(__SANITIZE_THREAD__) || PIKA_HAS_FEATURE(thread_sanitizer) # define PIKA_HAVE_THREAD_SANITIZER # if defined(PIKA_GCC_VERSION) || defined(PIKA_CLANG_VERSION) # define PIKA_NO_SANITIZE_THREAD __attribute__((no_sanitize("thread"))) From 48a992273dcf8bd48b98858a7fd21cdfe462b2b5 Mon Sep 17 00:00:00 2001 From: John Biddiscombe Date: Fri, 8 Nov 2024 17:13:27 +0100 Subject: [PATCH 03/14] Wrap mpi dispatch/trigger in let_value to ensure variable lifetime --- .../include/pika/async_mpi/transform_mpi.hpp | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp b/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp index 7fd6137c6..6663385c7 100644 --- a/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp +++ b/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp @@ -78,14 +78,23 @@ namespace pika::mpi::experimental { if (requests_inline) { - return dispatch_mpi_sender{PIKA_MOVE(sender), PIKA_FORWARD(F, f)} | - let_value(completion_snd); + return PIKA_FORWARD(Sender, sender) | + let_value([=, f = std::move(f)](auto&... args) { + auto snd0 = just(std::forward(args)...); + return dispatch_mpi_sender{ + PIKA_MOVE(snd0), PIKA_FORWARD(F, f)} | + let_value(completion_snd); + }); } else { - auto snd0 = PIKA_FORWARD(Sender, sender) | continues_on(mpi_pool_scheduler(p)); - return dispatch_mpi_sender{PIKA_MOVE(snd0), PIKA_FORWARD(F, f)} | - let_value(completion_snd); + return PIKA_FORWARD(Sender, sender) | continues_on(mpi_pool_scheduler(p)) | + let_value([=, f = std::move(f)](auto&... args) { + auto snd0 = just(std::forward(args)...); + return dispatch_mpi_sender{ + PIKA_MOVE(snd0), PIKA_FORWARD(F, f)} | + let_value(completion_snd); + }); } } From 1133b41d2de758f1e9e6779b20d6e85320be2dbd Mon Sep 17 00:00:00 2001 From: John Biddiscombe Date: Fri, 8 Nov 2024 23:50:12 +0100 Subject: [PATCH 04/14] Remove std::forward from let_value mpi transform wrapper --- libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp b/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp index 6663385c7..209654db3 100644 --- a/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp +++ b/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp @@ -80,7 +80,7 @@ namespace pika::mpi::experimental { { return PIKA_FORWARD(Sender, sender) | let_value([=, f = std::move(f)](auto&... args) { - auto snd0 = just(std::forward(args)...); + auto snd0 = just(args...); return dispatch_mpi_sender{ PIKA_MOVE(snd0), PIKA_FORWARD(F, f)} | let_value(completion_snd); @@ -90,7 +90,7 @@ namespace pika::mpi::experimental { { return PIKA_FORWARD(Sender, sender) | continues_on(mpi_pool_scheduler(p)) | let_value([=, f = std::move(f)](auto&... args) { - auto snd0 = just(std::forward(args)...); + auto snd0 = just(args...); return dispatch_mpi_sender{ PIKA_MOVE(snd0), PIKA_FORWARD(F, f)} | let_value(completion_snd); From 14ac430c156790f44d55ce6f695429551d2ca1f8 Mon Sep 17 00:00:00 2001 From: John Biddiscombe Date: Mon, 11 Nov 2024 10:18:56 +0100 Subject: [PATCH 05/14] Pass by value to test arg lifetime compilation fix --- libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp b/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp index 209654db3..a982aa4db 100644 --- a/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp +++ b/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp @@ -79,7 +79,7 @@ namespace pika::mpi::experimental { if (requests_inline) { return PIKA_FORWARD(Sender, sender) | - let_value([=, f = std::move(f)](auto&... args) { + let_value([=, f = std::move(f)](auto... args) { auto snd0 = just(args...); return dispatch_mpi_sender{ PIKA_MOVE(snd0), PIKA_FORWARD(F, f)} | @@ -89,7 +89,7 @@ namespace pika::mpi::experimental { else { return PIKA_FORWARD(Sender, sender) | continues_on(mpi_pool_scheduler(p)) | - let_value([=, f = std::move(f)](auto&... args) { + let_value([=, f = std::move(f)](auto... args) { auto snd0 = just(args...); return dispatch_mpi_sender{ PIKA_MOVE(snd0), PIKA_FORWARD(F, f)} | From 2dbc999ef52b974fd328b9ecbde4816ef137c31c Mon Sep 17 00:00:00 2001 From: John Biddiscombe Date: Mon, 11 Nov 2024 10:31:23 +0100 Subject: [PATCH 06/14] Forward function f though mpi let value wrapper --- libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp b/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp index a982aa4db..51048a73a 100644 --- a/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp +++ b/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp @@ -79,7 +79,7 @@ namespace pika::mpi::experimental { if (requests_inline) { return PIKA_FORWARD(Sender, sender) | - let_value([=, f = std::move(f)](auto... args) { + let_value([=, f = std::forward(f)](auto&... args) { auto snd0 = just(args...); return dispatch_mpi_sender{ PIKA_MOVE(snd0), PIKA_FORWARD(F, f)} | @@ -89,7 +89,7 @@ namespace pika::mpi::experimental { else { return PIKA_FORWARD(Sender, sender) | continues_on(mpi_pool_scheduler(p)) | - let_value([=, f = std::move(f)](auto... args) { + let_value([=, f = std::forward(f)](auto&... args) { auto snd0 = just(args...); return dispatch_mpi_sender{ PIKA_MOVE(snd0), PIKA_FORWARD(F, f)} | From c00aea3c0814190347780016a6e1c7837bc0d173 Mon Sep 17 00:00:00 2001 From: John Biddiscombe Date: Mon, 11 Nov 2024 11:15:38 +0100 Subject: [PATCH 07/14] Capture by reference, make let_value lambda mutable, move function into mpi --- .../include/pika/async_mpi/transform_mpi.hpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp b/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp index 51048a73a..fcea93afb 100644 --- a/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp +++ b/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp @@ -78,21 +78,21 @@ namespace pika::mpi::experimental { if (requests_inline) { - return PIKA_FORWARD(Sender, sender) | - let_value([=, f = std::forward(f)](auto&... args) { + return std::forward(sender) | + let_value([=, f = std::forward(f)](auto&... args) mutable { auto snd0 = just(args...); return dispatch_mpi_sender{ - PIKA_MOVE(snd0), PIKA_FORWARD(F, f)} | + std::move(snd0), std::move(f)} | let_value(completion_snd); }); } else { - return PIKA_FORWARD(Sender, sender) | continues_on(mpi_pool_scheduler(p)) | - let_value([=, f = std::forward(f)](auto&... args) { + return std::forward(sender) | continues_on(mpi_pool_scheduler(p)) | + let_value([=, f = std::forward(f)](auto&... args) mutable { auto snd0 = just(args...); return dispatch_mpi_sender{ - PIKA_MOVE(snd0), PIKA_FORWARD(F, f)} | + std::move(snd0), std::move(f)} | let_value(completion_snd); }); } From 5977b3d66e5e080edc12afd17f734862f49fde65 Mon Sep 17 00:00:00 2001 From: John Biddiscombe Date: Mon, 11 Nov 2024 16:25:56 +0100 Subject: [PATCH 08/14] Allow move only objects to be used (and kept alive) in transform_mpi Add test for move only transform_mpi --- .../include/pika/async_mpi/transform_mpi.hpp | 10 +++++++--- .../async_mpi/tests/unit/algorithm_transform_mpi.cpp | 11 +++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp b/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp index fcea93afb..1c02b0fd8 100644 --- a/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp +++ b/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -52,6 +53,7 @@ namespace pika::mpi::experimental { using pika::execution::experimental::just; using pika::execution::experimental::let_value; using pika::execution::experimental::unique_any_sender; + using pika::execution::experimental::unpack; // get mpi completion mode settings auto mode = get_completion_mode(); @@ -79,8 +81,9 @@ namespace pika::mpi::experimental { if (requests_inline) { return std::forward(sender) | - let_value([=, f = std::forward(f)](auto&... args) mutable { - auto snd0 = just(args...); + let_value([=, f = std::forward(f)](auto&&... args) mutable { + std::tuple ts{args...}; + auto snd0 = just(ts) | ex::unpack(); return dispatch_mpi_sender{ std::move(snd0), std::move(f)} | let_value(completion_snd); @@ -90,7 +93,8 @@ namespace pika::mpi::experimental { { return std::forward(sender) | continues_on(mpi_pool_scheduler(p)) | let_value([=, f = std::forward(f)](auto&... args) mutable { - auto snd0 = just(args...); + std::tuple ts{args...}; + auto snd0 = just(ts) | ex::unpack(); return dispatch_mpi_sender{ std::move(snd0), std::move(f)} | let_value(completion_snd); diff --git a/libs/pika/async_mpi/tests/unit/algorithm_transform_mpi.cpp b/libs/pika/async_mpi/tests/unit/algorithm_transform_mpi.cpp index 3b030dbaf..c79684f39 100644 --- a/libs/pika/async_mpi/tests/unit/algorithm_transform_mpi.cpp +++ b/libs/pika/async_mpi/tests/unit/algorithm_transform_mpi.cpp @@ -212,6 +212,17 @@ int pika_main() tt::sync_wait(PIKA_MOVE(s)); } + { + auto s = ex::just(custom_type_non_default_constructible_non_copyable{42}, datatype, + 0, comm) | + ex::drop_operation_state() | + mpi::transform_mpi([](auto& data, MPI_Datatype datatype, int i, MPI_Comm comm, + MPI_Request* request) { + MPI_Ibcast(&data, 1, datatype, i, comm, request); + }); + tt::sync_wait(PIKA_MOVE(s)); + } + // transform_mpi should be able to handle reference types (by copying // them to the operation state) { From 98d7e594e4b47b69effcb9fcf1202e2fd3b07ccb Mon Sep 17 00:00:00 2001 From: Mikael Simberg Date: Wed, 13 Nov 2024 18:00:09 +0100 Subject: [PATCH 09/14] Simplify sender pipeline in transform_mpi --- .../include/pika/async_mpi/transform_mpi.hpp | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp b/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp index 1c02b0fd8..fef645d5f 100644 --- a/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp +++ b/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp @@ -82,22 +82,16 @@ namespace pika::mpi::experimental { { return std::forward(sender) | let_value([=, f = std::forward(f)](auto&&... args) mutable { - std::tuple ts{args...}; - auto snd0 = just(ts) | ex::unpack(); - return dispatch_mpi_sender{ - std::move(snd0), std::move(f)} | - let_value(completion_snd); + return just(std::tuple{args...}) | ex::unpack() | + dispatch_mpi(std::move(f)) | let_value(completion_snd); }); } else { return std::forward(sender) | continues_on(mpi_pool_scheduler(p)) | let_value([=, f = std::forward(f)](auto&... args) mutable { - std::tuple ts{args...}; - auto snd0 = just(ts) | ex::unpack(); - return dispatch_mpi_sender{ - std::move(snd0), std::move(f)} | - let_value(completion_snd); + return just(std::tuple{args...}) | ex::unpack() | + dispatch_mpi(std::move(f)) | let_value(completion_snd); }); } } From 7b57635f6283b3828ae5dc0f7a856dd8b25308bf Mon Sep 17 00:00:00 2001 From: Mikael Simberg Date: Wed, 13 Nov 2024 18:00:22 +0100 Subject: [PATCH 10/14] Explicitly use l-value reference instead of forwarding reference in transform_mpi let_value callable --- libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp b/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp index fef645d5f..b2fc7bc70 100644 --- a/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp +++ b/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp @@ -81,7 +81,7 @@ namespace pika::mpi::experimental { if (requests_inline) { return std::forward(sender) | - let_value([=, f = std::forward(f)](auto&&... args) mutable { + let_value([=, f = std::forward(f)](auto&... args) mutable { return just(std::tuple{args...}) | ex::unpack() | dispatch_mpi(std::move(f)) | let_value(completion_snd); }); From 6147b21ba8e6e43da16b4941b4e2728acbb917e7 Mon Sep 17 00:00:00 2001 From: Mikael Simberg Date: Wed, 13 Nov 2024 19:57:32 +0100 Subject: [PATCH 11/14] Pass correct struct member to MPI_Ibcast --- libs/pika/async_mpi/tests/unit/algorithm_transform_mpi.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/pika/async_mpi/tests/unit/algorithm_transform_mpi.cpp b/libs/pika/async_mpi/tests/unit/algorithm_transform_mpi.cpp index c79684f39..f4f0f483b 100644 --- a/libs/pika/async_mpi/tests/unit/algorithm_transform_mpi.cpp +++ b/libs/pika/async_mpi/tests/unit/algorithm_transform_mpi.cpp @@ -218,7 +218,7 @@ int pika_main() ex::drop_operation_state() | mpi::transform_mpi([](auto& data, MPI_Datatype datatype, int i, MPI_Comm comm, MPI_Request* request) { - MPI_Ibcast(&data, 1, datatype, i, comm, request); + MPI_Ibcast(&data.x, 1, datatype, i, comm, request); }); tt::sync_wait(PIKA_MOVE(s)); } From 329ef2d602183aee394c605506bf617148cce698 Mon Sep 17 00:00:00 2001 From: Mikael Simberg Date: Wed, 13 Nov 2024 20:00:00 +0100 Subject: [PATCH 12/14] Remove unnecessary unique_any_sender from dispatch_mpi adaptor --- libs/pika/async_mpi/include/pika/async_mpi/dispatch_mpi.hpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libs/pika/async_mpi/include/pika/async_mpi/dispatch_mpi.hpp b/libs/pika/async_mpi/include/pika/async_mpi/dispatch_mpi.hpp index 27ca26250..71b9366cd 100644 --- a/libs/pika/async_mpi/include/pika/async_mpi/dispatch_mpi.hpp +++ b/libs/pika/async_mpi/include/pika/async_mpi/dispatch_mpi.hpp @@ -237,9 +237,8 @@ namespace pika::mpi::experimental { friend constexpr PIKA_FORCEINLINE auto tag_fallback_invoke(dispatch_mpi_t, Sender&& sender, F&& f) { - auto snd1 = detail::dispatch_mpi_sender{ + return detail::dispatch_mpi_sender{ PIKA_FORWARD(Sender, sender), PIKA_FORWARD(F, f)}; - return pika::execution::experimental::make_unique_any_sender(std::move(snd1)); } template From 3afc0a7148c542357ae183c8def4d28991de6f34 Mon Sep 17 00:00:00 2001 From: Mikael Simberg Date: Wed, 13 Nov 2024 20:18:41 +0100 Subject: [PATCH 13/14] Add test to make sure transform_mpi does not pass references to values sent from previous senders --- libs/pika/async_mpi/tests/unit/algorithm_transform_mpi.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/libs/pika/async_mpi/tests/unit/algorithm_transform_mpi.cpp b/libs/pika/async_mpi/tests/unit/algorithm_transform_mpi.cpp index f4f0f483b..dd69e2ff2 100644 --- a/libs/pika/async_mpi/tests/unit/algorithm_transform_mpi.cpp +++ b/libs/pika/async_mpi/tests/unit/algorithm_transform_mpi.cpp @@ -228,9 +228,10 @@ int pika_main() { int data = 0, count = 1; if (rank == 0) { data = 42; } - auto s = mpi::transform_mpi( - const_reference_sender{count}, [&](int& count, MPI_Request* request) { - MPI_Ibcast(&data, count, datatype, 0, comm, request); + auto s = mpi::transform_mpi(const_reference_sender{count}, + [&](int& count_transform_mpi, MPI_Request* request) { + PIKA_TEST(&count_transform_mpi != &count); + MPI_Ibcast(&data, count_transform_mpi, datatype, 0, comm, request); }); tt::sync_wait(PIKA_MOVE(s)); PIKA_TEST_EQ(data, 42); From 18b0162cce70305560517368ec9a5755f7ea2852 Mon Sep 17 00:00:00 2001 From: Mikael Simberg Date: Wed, 13 Nov 2024 20:40:01 +0100 Subject: [PATCH 14/14] Use forward_as_tuple to forward references in transform_mpi --- libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp b/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp index b2fc7bc70..7472b6925 100644 --- a/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp +++ b/libs/pika/async_mpi/include/pika/async_mpi/transform_mpi.hpp @@ -82,7 +82,7 @@ namespace pika::mpi::experimental { { return std::forward(sender) | let_value([=, f = std::forward(f)](auto&... args) mutable { - return just(std::tuple{args...}) | ex::unpack() | + return just(std::forward_as_tuple(args...)) | ex::unpack() | dispatch_mpi(std::move(f)) | let_value(completion_snd); }); } @@ -90,7 +90,7 @@ namespace pika::mpi::experimental { { return std::forward(sender) | continues_on(mpi_pool_scheduler(p)) | let_value([=, f = std::forward(f)](auto&... args) mutable { - return just(std::tuple{args...}) | ex::unpack() | + return just(std::forward_as_tuple(args...)) | ex::unpack() | dispatch_mpi(std::move(f)) | let_value(completion_snd); }); }