Topic/tl ucp kn algs #361

vspetrov · 2021-12-06T10:35:55Z

What

TL/UCP
Adds implementation of knomial reduce-scatter, allgather, reduce-scatterv, allgatherv
Adds ring reduce_scatter
Selection between algs
Tests

Why ?

Generalize existing knomial pattern for other use-cases - expose to user (not only for sra kn allreduce)

ReduceScatter Ring Bi- vs Uni- directional data: host mem, 8 jazz nodes, ppn=1:

[~/workspace/ucc/build_rel]
[valentinp@hpchead]> mpirun -x UCX_NET_DEVICES=mlx5_0:1 -x UCC_TL_UCP_REDUCE_SCATTER_RING_BIDIRECTIONAL=y -x UCC_TL_UCP_TUNE=reduce_scatter:@ring -x UCC_TLS=ucp -np 8 --map-by node ./install/bin/ucc_perftest -c reduce_scatter -b 8192 -e $((2**20))
Collective:             ReduceScatter
Memory type:            host
Datatype:               float32
Reduction:              sum
Inplace:                0
Warmup:
  small                 100
  large                 20
Iterations:
  small                 1000
  large                 200

       Count        Size                Time, us
                                 avg         min         max
        8192       32768       53.07       52.87       53.31
       16384       65536       86.33       85.85       87.37
       32768      131072      147.08      146.04      149.74
       65536      262144      271.16      270.32      272.31
      131072      524288      544.31      543.22      546.50
      262144     1048576     1133.39     1121.06     1140.96
      524288     2097152     2328.61     2311.57     2344.70
     1048576     4194304     4800.29     4766.70     4815.62

[~/workspace/ucc/build_rel]
[valentinp@hpchead]> mpirun -x UCX_NET_DEVICES=mlx5_0:1 -x UCC_TL_UCP_REDUCE_SCATTER_RING_BIDIRECTIONAL=n -x UCC_TL_UCP_TUNE=reduce_scatter:@ring -x UCC_TLS=ucp -np 8 --map-by node ./install/bin/ucc_perftest -c reduce_scatter -b 8192 -e $((2**20))
Collective:             ReduceScatter
Memory type:            host
Datatype:               float32
Reduction:              sum
Inplace:                0
Warmup:
  small                 100
  large                 20
Iterations:
  small                 1000
  large                 200

       Count        Size                Time, us
                                 avg         min         max
        8192       32768       66.90       66.21       67.46
       16384       65536      103.88      100.91      108.14
       32768      131072      175.62      167.65      183.85
       65536      262144      305.82      304.03      307.39
      131072      524288      626.65      601.01      637.28
      262144     1048576     1254.95     1228.15     1288.78
      524288     2097152     2546.16     2505.00     2576.32
     1048576     4194304     5181.85     5131.23     5211.40

Sergei-Lebedev · 2021-12-22T08:05:36Z

src/components/tl/ucp/allgather/allgather_knomial.c

-        broot = coll_args->args.root;
-        rank = VRANK(rank, broot, size);
+    if (coll_args->args.coll_type == UCC_COLL_TYPE_ALLGATHER) {
+        ucc_kn_ag_pattern_init(size, rank, radix,


pattern init is also part of ucc_tl_ucp_allgather_knomial_start function, do we really need to do it twice?

looks you are right - not needed here. copy-pasted from reduce-scatter knomial. there pattern init is called there because i need to understand which ranks are extra/proxy - this info is part of kn pattern

src/components/tl/ucp/tl_ucp_coll.c

Sergei-Lebedev · 2021-12-22T08:44:59Z

src/components/tl/ucp/allgather/allgather_knomial.c

+    } else {
+        count = args->dst.info.count;
+        start = NULL;
+        ucc_kn_agx_pattern_init(size, rank, radix, count,


don't you need to use VRANK(rank) here for sag bcast?

true - it is done above at line 201.

Sergei-Lebedev · 2021-12-22T09:14:17Z

src/components/tl/ucp/reduce_scatter/reduce_scatter_ring.c

+        prevblock =
+            ucc_ep_map_eval(task->reduce_scatter_ring.inv_map, prevblock);
+        if (task->recv_completed == size - 1) {
+            ucc_assert(prevblock == UCC_TL_TEAM_RANK(team));


what is the purpose of this assert? first checking prevblock is rank and assign to rank in next line

correct - removed (part of debug)

Sergei-Lebedev · 2021-12-22T09:27:11Z

src/components/tl/ucp/reduce_scatter/reduce_scatter_ring.c

+                                         team, task),
+                      task, out);
+
+        recv_data_from = (rank - 2 - step + size) % size;


looks like this value is negative for rank 0 on last step
last step recv_data_from = (rank - 2 - (size - 1) + size) % size = (rank - 1) % size

it never gets there. on last step the loop will break at line 132.

Sergei-Lebedev · 2021-12-22T09:28:45Z

src/components/tl/ucp/reduce_scatter/reduce_scatter_ring.c

+    }
+
+    max_segcount = ucc_buffer_block_count(count, size, 0);
+    //TODO need less mem - dvivide by n_frags


why not to divide?

Sergei-Lebedev · 2021-12-22T09:39:11Z

src/components/tl/ucp/reduce_scatter/reduce_scatter_ring.c

+    max_segcount = ucc_buffer_block_count(count, size, 0);
+    //TODO need less mem - dvivide by n_frags
+    to_alloc = max_segcount + (size > 2 ? 2 * max_segcount : 0);
+    status   = ucc_mc_alloc(&task->reduce_scatter_ring.scratch_mc_header,


I think it's better to allocate one chunk of memory and give portion of it to each subset rather than allow each subset allocate it's own scratch

agree. done.

Sergei-Lebedev · 2021-12-22T09:47:57Z

src/components/tl/ucp/tl_ucp.c

     "else - in result",
     ucc_offsetof(ucc_tl_ucp_lib_config_t, reduce_avg_pre_op),
     UCC_CONFIG_TYPE_BOOL},

+    {"REDUCE_SCATTER_RING_BIDIRECTIONAL", "y",


do you have some perf data of unidirectional vs bidirectional? should we add bidirectional implementation for other ring algorithms e.g. allgather?

added to PR desc. Maybe it worth extending ag as well at some point. It is not always beneficial (e.g., if ring covers CPU SHM then there is no gain, since shm is not bibw).

Sergei-Lebedev · 2021-12-22T09:51:07Z

src/utils/ucc_coll_utils.c

+        }
+        for (i = 0; i < map.ep_num; i++) {
+            r = (ucc_rank_t)ucc_ep_map_eval(map, i);
+            *((ucc_rank_t *)PTR_OFFSET(inv.array.map, sizeof(ucc_rank_t) * r)) =


why not just inv.array.map[r] = i ?

map is void* inside array (since we support it to be 32 and 64 bits)

Sergei-Lebedev · 2021-12-22T09:56:43Z

test/mpi/test_reduce_scatterv.cc

+            return status;
+        }
+    }
+    if (inplace) {


done. remove currently. will add back, when we add MPI-like inplace support for RS and RSv.

src/coll_patterns/recursive_knomial.h

src/components/tl/ucp/reduce_scatter/reduce_scatter_ring.c

src/coll_patterns/sra_knomial.h

shimmybalsam · 2021-12-28T14:10:50Z

src/components/tl/ucp/reduce_scatter/reduce_scatter_knomial.c

+        _buf;                                                                  \
+    })
+
+#define GET_DT(_args)                                                          \


these functions seem repetitive to what you did in allgather_knomial.c - perhaps make a more general function in tl_ucp_coll.h?

adjust count depending on comm size add reduce_scatterV

vspetrov · 2022-01-13T18:47:11Z

bot:retest

vspetrov · 2022-02-01T18:14:56Z

i will split PR into smaller pieces

vspetrov mentioned this pull request Dec 6, 2021

TL/UCP: reduce_scatter #353

Closed

vspetrov force-pushed the topic/tl_ucp_kn_algs branch 6 times, most recently from 622a074 to e9fdc15 Compare December 13, 2021 08:33

vspetrov force-pushed the topic/tl_ucp_kn_algs branch from e9fdc15 to ff59aa2 Compare December 20, 2021 12:09

vspetrov added the Ready-for-Review label Dec 20, 2021

vspetrov requested review from Sergei-Lebedev, shimmybalsam, bureddy and manjugv December 20, 2021 12:30

Sergei-Lebedev reviewed Dec 22, 2021

View reviewed changes

shimmybalsam reviewed Dec 28, 2021

View reviewed changes

vspetrov force-pushed the topic/tl_ucp_kn_algs branch 6 times, most recently from b28fb4e to 3d1bd26 Compare January 13, 2022 15:20

Valentin Petrov added 9 commits January 13, 2022 19:40

UTIL: new knomial pattern implementation

ba65fb0

TL/UCP: adapt to new kn pattern

5d77a4f

TL/UCP: ring reduce-scatter

3a9c5af

TL/UCP: selection of kn algs

61aa547

TEST: reduce scatter in gtest

6d9b6cd

TEST: mpi reduce scatter v

df6bb19

TEST: mpi reduce_scatter

63dacc5

adjust count depending on comm size add reduce_scatterV

TL/UCP: rs kn buffers init

43e9f86

UTIL: fix inv map

3113dbb

Valentin Petrov added 3 commits January 13, 2022 19:40

TL/UCP: allgatherv kn contig dst

f35eadb

CODESTYLE: apply clang

cc0d719

UTIL: fix knx pattern

50e800b

vspetrov force-pushed the topic/tl_ucp_kn_algs branch from 3d1bd26 to 99d3a68 Compare January 13, 2022 17:40

Valentin Petrov added 2 commits January 13, 2022 21:16

TEST: fix RSv test after rebase

c863e64

REVIEW: address comments

9213fae

vspetrov force-pushed the topic/tl_ucp_kn_algs branch from 99d3a68 to 9213fae Compare January 13, 2022 19:19

vspetrov requested a review from Sergei-Lebedev January 13, 2022 20:55

vspetrov added WIP - Don't Merge and removed Ready-for-Review labels Feb 1, 2022

vspetrov mentioned this pull request Feb 1, 2022

TL/UCP: reduce_scatter_ring #413

Merged

Sergei-Lebedev mentioned this pull request Feb 13, 2023

TL/UCP: add knomial allgather #729

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Topic/tl ucp kn algs #361

Topic/tl ucp kn algs #361

vspetrov commented Dec 6, 2021 •

edited

Sergei-Lebedev Dec 22, 2021

vspetrov Jan 13, 2022

Sergei-Lebedev Dec 22, 2021

vspetrov Jan 13, 2022

Sergei-Lebedev Dec 22, 2021

vspetrov Jan 13, 2022

Sergei-Lebedev Dec 22, 2021

vspetrov Jan 13, 2022

Sergei-Lebedev Dec 22, 2021

vspetrov Jan 13, 2022

Sergei-Lebedev Dec 22, 2021

vspetrov Jan 13, 2022

Sergei-Lebedev Dec 22, 2021

vspetrov Jan 13, 2022

Sergei-Lebedev Dec 22, 2021

vspetrov Jan 13, 2022

Sergei-Lebedev Dec 22, 2021

vspetrov Jan 13, 2022

shimmybalsam Dec 28, 2021

vspetrov commented Jan 13, 2022

vspetrov commented Feb 1, 2022

Topic/tl ucp kn algs #361

Are you sure you want to change the base?

Topic/tl ucp kn algs #361

Conversation

vspetrov commented Dec 6, 2021 • edited

What

Why ?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vspetrov commented Jan 13, 2022

vspetrov commented Feb 1, 2022

vspetrov commented Dec 6, 2021 •

edited