ARCH/X86: Add AVX512 nt-buffer-transfer functions - #11954
arun-chandran-edarath wants to merge 1 commit into
Conversation
Add AVX512BW kernels with masked edge handling and compile-time ISA selection. Keep the AVX2 implementation as the fallback when AVX512BW is not enabled. Signed-off-by: Arun Chandran <Arun.Chandran@amd.com>
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| #ifdef __AVX__ | ||
| #if defined(__AVX512BW__) | ||
| #include "cpu_nt_avx512.inl" |
There was a problem hiding this comment.
nothing in the build ever adds -mavx512bw — config/m4/compiler.m4 only has COMPILER_CPU_OPTIMIZATION([avx], ...), and no job in buildlib//contrib/ passes --with-avx/--enable-optimizations/-march=native. so this branch and the whole new .inl are never compiled, and test_arch.nt_buffer_transfer_* never covers them. can we add an avx512bw optimization knob plus a CI build that enables it?
There was a problem hiding this comment.
@tvegas1 @roiedanino Any suggestions on how to handle this?
| } | ||
|
|
||
| #ifdef __AVX__ | ||
| #if defined(__AVX512BW__) |
There was a problem hiding this comment.
on Intel Skylake-SP/Cascade Lake, 512-bit stores trigger AVX-512 frequency licensing, so a -march=native build there may end up slower than the AVX2 path. should the AVX-512 kernels be picked by vendor/uarch instead of purely by __AVX512BW__?
| ucs_memory_bus_store_fence(); | ||
| } | ||
|
|
||
| /* AVX-512 NT_SOURCE copy-out: body stores, descending. An unconditional |
There was a problem hiding this comment.
why descending here? ucs_x86_nt_src_avx2_buffer_transfer copies ascending, and the descent is the only reason for the four _bwd helpers — the same masked-head + ascending-body structure as ucs_x86_nt_dst_avx512_buffer_transfer would drop all of them.
|
|
||
| /* Vector copy helpers. Ascending helpers store from the base [dst, dst+n); | ||
| * descending (_bwd) helpers store from the top [dst-n, dst). The | ||
| * ucs_compiler_fence() between the four stores pins their order (they are |
There was a problem hiding this comment.
why is the store ordering needed here when cpu_nt_avx2.inl has no fences at all? each 512-bit store already covers a full 64B line, so write-combining should not depend on the order, and in the _bwd helpers these are plain WB stores where the barrier only constrains the scheduler.
| arch/ppc64/cpu.h \ | ||
| arch/rv64/cpu.h \ | ||
| arch/x86_64/cpu.h \ | ||
| arch/x86_64/cpu_nt_avx512.inl \ |
There was a problem hiding this comment.
minor: pls keep the list sorted.
| arch/x86_64/cpu_nt_avx512.inl \ | |
| arch/x86_64/cpu_nt_avx2.inl \ | |
| arch/x86_64/cpu_nt_avx512.inl \ |
| static UCS_F_ALWAYS_INLINE void | ||
| ucs_x86_avx512_nt_masked_head_64(void *dst, const void *src, size_t edge_len) | ||
| { | ||
| const size_t head_pad = 64u - edge_len; |
There was a problem hiding this comment.
minor: -head_pad on a size_t wraps to 2^64 - head_pad and then relies on the implementation-defined (intptr_t) conversion in UCS_PTR_BYTE_OFFSET.
| const size_t head_pad = 64u - edge_len; | |
| const ptrdiff_t head_pad = 64 - (ptrdiff_t)edge_len; | |
| const __mmask64 k = (__mmask64)(~0ull << head_pad); |
|
@roiedanino @tvegas1 Please review. |
2 participants
What?
Add AVX512BW non-temporal destination and source kernels with masked edge handling. Select the AVX-512 implementation at compile time when
__AVX512BW__is defined and retain AVX2 as the fallback.This is the third part of the split from #11696, following #11878 and #11895.
Why?
Use the full AVX-512 vector width for large cache-bypassing transfers and use byte masks for non-cache-line-aligned edges.
How?
The NT destination kernel copies forward with streaming stores and an
sfence. The NT source kernel copies backward with regular stores and no fence. Both implementations retain the existing dispatcher contract.Build
AVX2: configure with --enable-optimizations.
AVX-512: add -march=znver5 or another target flag defining AVX512BW into CFLAGS.
Runtime requirements
Currently recommended for workloads using one MPI rank per L3 cache domain.
Required runtime arguments(config params):
UCX_NT_BUFFER_TRANSFER_MIN=0
The default UCX_NT_BUFFER_TRANSFER_MIN start threshold is three-quarters of the detected L3 cache size.