First pass at scan, implemented linear and ring algorithms #1154

avincigu · 2024-10-14T18:59:37Z

No description provided.

davidozog

This looks great, thanks @avincigu!

I noticed an issue with "in-place" scan (the specification example uses the same source and dest buffer). Do you have any other testing in place?

The C11 variants can probably added as well - something like this:

diff --git a/mpp/shmemx.h4 b/mpp/shmemx.h4
index 92b38519..f82729e0 100644
--- a/mpp/shmemx.h4
+++ b/mpp/shmemx.h4
@@ -119,6 +119,19 @@ SHMEM_BIND_C11_RMA(`SHMEM_C11_GEN_IBGET', `, \') \
         uint64_t*: shmemx_signal_add \
     )(__VA_ARGS__)

+define(`SHMEM_C11_GEN_EXSCAN', `        $2*: shmemx_$1_sum_exscan')dnl
+#define shmemx_sum_exscan(...) \
+    _Generic(SHMEM_C11_TYPE_EVAL_PTR(SHMEM_C11_ARG1(__VA_ARGS__)), \
+SHMEM_BIND_C11_RMA(`SHMEM_C11_GEN_EXSCAN', `, \') \
+    )(__VA_ARGS__)
+
+define(`SHMEM_C11_GEN_INSCAN', `        $2*: shmemx_$1_sum_inscan')dnl
+#define shmemx_sum_inscan(...) \
+    _Generic(SHMEM_C11_TYPE_EVAL_PTR(SHMEM_C11_ARG1(__VA_ARGS__)), \
+SHMEM_BIND_C11_RMA(`SHMEM_C11_GEN_INSCAN', `, \') \
+    )(__VA_ARGS__)

The code style is a bit different from typical SOS, at least in collectives.c. We gravitate towards the Google style guide:
https://google.github.io/styleguide/cppguide.html

Let's chat offline tomorrow, thanks!

src/collectives.c

davidozog · 2024-10-15T13:58:41Z

The Portals4 testing row looks like a real issue to me, seemingly with the pshmem symbols.
The UCX rows look like a known issue #1148

wrrobin · 2024-10-16T14:30:35Z

This looks great, thanks @avincigu!

I noticed an issue with "in-place" scan (the specification example uses the same source and dest buffer). Do you have any other testing in place?

The C11 variants can probably added as well - something like this:
diff --git a/mpp/shmemx.h4 b/mpp/shmemx.h4
index 92b38519..f82729e0 100644
--- a/mpp/shmemx.h4
+++ b/mpp/shmemx.h4
@@ -119,6 +119,19 @@ SHMEM_BIND_C11_RMA(`SHMEM_C11_GEN_IBGET', `, \') \
         uint64_t*: shmemx_signal_add \
     )(__VA_ARGS__)

+define(`SHMEM_C11_GEN_EXSCAN', `        $2*: shmemx_$1_sum_exscan')dnl
+#define shmemx_sum_exscan(...) \
+    _Generic(SHMEM_C11_TYPE_EVAL_PTR(SHMEM_C11_ARG1(__VA_ARGS__)), \
+SHMEM_BIND_C11_RMA(`SHMEM_C11_GEN_EXSCAN', `, \') \
+    )(__VA_ARGS__)
+
+define(`SHMEM_C11_GEN_INSCAN', `        $2*: shmemx_$1_sum_inscan')dnl
+#define shmemx_sum_inscan(...) \
+    _Generic(SHMEM_C11_TYPE_EVAL_PTR(SHMEM_C11_ARG1(__VA_ARGS__)), \
+SHMEM_BIND_C11_RMA(`SHMEM_C11_GEN_INSCAN', `, \') \
+    )(__VA_ARGS__)
The code style is a bit different from typical SOS, at least in collectives.c. We gravitate towards the Google style guide: https://google.github.io/styleguide/cppguide.html

Let's chat offline tomorrow, thanks!

Looking at the PR, I realize why @davidozog commented on code style. I did not see any issue when we were looking at the code @avincigu. Perhaps, it is the editor you were using.

avincigu · 2024-10-16T19:40:38Z

This looks great, thanks @avincigu!

I noticed an issue with "in-place" scan (the specification example uses the same source and dest buffer). Do you have any other testing in place?

The C11 variants can probably added as well - something like this:
diff --git a/mpp/shmemx.h4 b/mpp/shmemx.h4
index 92b38519..f82729e0 100644
--- a/mpp/shmemx.h4
+++ b/mpp/shmemx.h4
@@ -119,6 +119,19 @@ SHMEM_BIND_C11_RMA(`SHMEM_C11_GEN_IBGET', `, \') \
         uint64_t*: shmemx_signal_add \
     )(__VA_ARGS__)

+define(`SHMEM_C11_GEN_EXSCAN', `        $2*: shmemx_$1_sum_exscan')dnl
+#define shmemx_sum_exscan(...) \
+    _Generic(SHMEM_C11_TYPE_EVAL_PTR(SHMEM_C11_ARG1(__VA_ARGS__)), \
+SHMEM_BIND_C11_RMA(`SHMEM_C11_GEN_EXSCAN', `, \') \
+    )(__VA_ARGS__)
+
+define(`SHMEM_C11_GEN_INSCAN', `        $2*: shmemx_$1_sum_inscan')dnl
+#define shmemx_sum_inscan(...) \
+    _Generic(SHMEM_C11_TYPE_EVAL_PTR(SHMEM_C11_ARG1(__VA_ARGS__)), \
+SHMEM_BIND_C11_RMA(`SHMEM_C11_GEN_INSCAN', `, \') \
+    )(__VA_ARGS__)
The code style is a bit different from typical SOS, at least in collectives.c. We gravitate towards the Google style guide: https://google.github.io/styleguide/cppguide.html

Let's chat offline tomorrow, thanks!

Thanks for the comments Dave! I made the changes you suggested. It seems like my editor was adding extra spaces whenever I hit tab. I have fixed it now, please let me know if you see any style issues still.

wrrobin · 2024-12-13T15:20:47Z

@avincigu - XPMEM tests are failing. You might have to try those configs to debug and see what is the issue. Neptune/Perlmutter has XPMEM as a module.

mpp/shmemx.h4

src/collectives.c

Co-authored-by: Md Rahman <wrrobin@gmail.com>

src/collectives.c

wrrobin

LGTM @avincigu. BTW, please add another reviewer to review unless we get @davidozog's approval.

davidozog

Looks great - I just pointed out a few minor things.

davidozog · 2025-01-17T18:02:17Z

mpp/shmemx.h4

+define(`SHMEM_C11_GEN_EXSCAN', `        $2*: shmemx_$1_sum_exscan')dnl
+#define shmemx_sum_exscan(...) \
+    _Generic(SHMEM_C11_TYPE_EVAL_PTR(SHMEM_C11_ARG1(__VA_ARGS__)), \
+SHMEM_BIND_C11_RMA(`SHMEM_C11_GEN_EXSCAN', `, \') \


I think these need to be SHMEM_BIND_C11_COLL_SUM_PROD.

davidozog · 2025-01-17T18:02:36Z

mpp/shmemx.h4

+define(`SHMEM_C11_GEN_INSCAN', `        $2*: shmemx_$1_sum_inscan')dnl
+#define shmemx_sum_inscan(...) \
+    _Generic(SHMEM_C11_TYPE_EVAL_PTR(SHMEM_C11_ARG1(__VA_ARGS__)), \
+SHMEM_BIND_C11_RMA(`SHMEM_C11_GEN_INSCAN', `, \') \


Here too - SHMEM_BIND_C11_COLL_SUM_PROD?

davidozog · 2025-01-17T18:20:20Z

src/collectives.c

@@ -18,13 +18,15 @@

 #define SHMEM_INTERNAL_INCLUDE
 #include "shmem.h"
+#include "shmemx.h"


Minor - I don't believe this is needed, it's in shmem_internal.h.

davidozog · 2025-01-17T18:27:14Z

src/collectives.c

+
+        /* initialize target buffer.  The put
+           will flush any atomic cache value that may currently
+           exist. */


Minor - this is slightly out-of-place (narrow) comment style here.

davidozog · 2025-01-17T18:28:04Z

src/collectives.c

+
+        /* initialize target buffer.  The put
+           will flush any atomic cache value that may currently
+           exist. */


Minor - slightly narrow comment style.

davidozog · 2025-01-17T18:38:37Z

src/shmem_collectives.h

+        case AUTO:
+            shmem_internal_scan_linear(target, source, count, type_size,
+                                            PE_start, PE_stride, PE_size,
+                                            pWrk, pSync, op, datatype, 1);


Minor style thing - SOS tends to align with the first argument's column, not to suggest that it's perfect everywhere ;) (see op_to_all above and collect_linear).

davidozog · 2025-01-17T18:43:05Z

src/collectives_c.c4

@@ -28,6 +28,7 @@ include(shmem_bind_c.m4)dnl

 #define SHMEM_INTERNAL_INCLUDE
 #include "shmem.h"
+#include "shmemx.h"


Minor - I don't believe this is needed, it's in shmem_internal.h.

avincigu added 2 commits October 14, 2024 14:52

First pass at scan, implemented linear and ring algorithms

833cd84

Adding ring value to san type

c301f99

avincigu requested review from davidozog and wrrobin October 14, 2024 21:05

davidozog reviewed Oct 15, 2024

View reviewed changes

src/collectives.c Outdated Show resolved Hide resolved

src/collectives.c Outdated Show resolved Hide resolved

Added code for special case of in place scan

9dc152c

Changes to style and format

60d1dac

avincigu added 2 commits December 18, 2024 10:50

Implementing shared transport for atomicv

a1c8631

Adding ring testing to scan

013731a

wrrobin reviewed Jan 6, 2025

View reviewed changes

mpp/shmemx.h4 Outdated Show resolved Hide resolved

wrrobin reviewed Jan 6, 2025

View reviewed changes

src/collectives.c Outdated Show resolved Hide resolved

avincigu and others added 6 commits January 6, 2025 15:29

Update mpp/shmemx.h4

98fd52d

Co-authored-by: Md Rahman <wrrobin@gmail.com>

Update src/collectives.c

32bd72d

Co-authored-by: Md Rahman <wrrobin@gmail.com>

Fix for XPMEM test failure

aa60eec

Merge branch 'scan' of https://github.com/avincigu/SOS into scan

caffaca

Fix for UCX tests

dd5fca9

Fixed style issues

e4f69db