ARCH/X86: Add AVX512 nt-buffer-transfer functions - #11696
arun-chandran-edarath wants to merge 10 commits into
Conversation
- Refactor AVX2 kernels into an ISA-specific inline fragment. - Add AVX-512 masked-edge NT source and destination kernels. - Share the ERMS copy helper and extend NT-path test coverage. Signed-off-by: Arun Chandran <Arun.Chandran@amd.com>
|
@roiedanino @yosefe @tvegas1 could you please help to review this? |
|
🤖 Starting review — findings will be posted here when done. |
|
PR size: the change removes ~430 lines and adds ~560 across the two |
Signed-off-by: Arun Chandran <Arun.Chandran@amd.com>
Setting NT_BUFFER_TRANSFER_MIN explicitly should let the NT dispatcher choose the copy implementation. Disable the outer ERMS window during CPU initialization and document the configuration effect. Signed-off-by: Arun Chandran <Arun.Chandran@amd.com>
Replace the literal 3072-byte cutoff with a local constant so the minimum per-chunk NT transfer size is explicit. Signed-off-by: Arun Chandran <Arun.Chandran@amd.com>
Run the existing NT source transfer coverage with the vector path forced and with ERMS enabled so both dispatcher branches execute. Signed-off-by: Arun Chandran <Arun.Chandran@amd.com>
Thanks for flagging this. I considered the suggested splits, but I would prefer to keep this as one PR. Although the added-line count exceeds the guidance, this is a self-contained rewrite of one buffer-transfer implementation. The AVX2 and AVX-512 kernels share the same dispatcher and test contract, and reviewing them together makes their behavioral parity clearer. Splitting out the ERMS helper would also create a preparatory change with no independent behavior. I have kept the follow-up fixes in focused commits and addressed the other review comments. Given the cohesive scope, I believe keeping this together is clearer than introducing an artificial split. |
Describe inner ERMS selection in terms of builtin_memcpy_min and explain how explicit NT configuration closes the outer window. Print NT thresholds as exclusive, matching their strict comparisons. Signed-off-by: Arun Chandran <Arun.Chandran@amd.com>
Compile the ERMS helper and NT source selection only when built-in memcpy is enabled. Skip ERMS-specific test coverage in disabled builds. Signed-off-by: Arun Chandran <Arun.Chandran@amd.com>
Signed-off-by: Arun Chandran <Arun.Chandran@amd.com>
|
@roiedanino @tvegas1 Gentle reminder to review this PR when you have a chance. Thanks! |
|
🤖 Starting review — findings will be posted here when done. |
| { | ||
| if (ucs_arch_get_cpu_vendor() == UCS_CPU_VENDOR_AMD) { | ||
| return ucs_cpu_get_cache_size(UCS_CPU_CACHE_L3) * 9 / 8; | ||
| return ucs_cpu_get_cache_size(UCS_CPU_CACHE_L3) * 3 / 4; |
There was a problem hiding this comment.
nt_dest_threshold change makes the NT_SOURCE path effectively unreachable on AMD. The PR lowers ucs_cpu_nt_dest_thresh() from L3 * 9/8 to L3 * 3/4. ucs_cpu_nt_bt_thresh_min() (auto) already returns L3 * 3/4, and ucs_memcpy_relaxed() only enters ucs_x86_nt_buffer_transfer() when total_len >= nt_buffer_transfer_min (L3*3/4). Inside, total_len > nt_dest_threshold (L3*3/4) now forces hint = UCS_ARCH_MEMCPY_NT_DEST. So for all total_len > L3*3/4 the NT_SOURCE hint is overridden by NT_DEST — the new source copy-out path only runs at the single boundary value total_len == L3*3/4. Is the source copy-out path meant to be reachable on AMD, or is it now effectively dead?
There was a problem hiding this comment.
Yes, this is intentional. With the default auto setting, your analysis is correct: NT_SOURCE is effectively reachable only at the boundary value.
The default thresholds are conservative to avoid degrading performance when multiple ranks share an L3 cache. For placements where an L3 is not shared by multiple ranks—for example, one rank per L3, NUMA domain, or socket—the user can enable this path by setting UCX_NT_BUFFER_TRANSFER_MIN=0.
With that setting, the supplied hint is preserved when total_len is less than or equal to 3/4 of L3, allowing UCS_ARCH_MEMCPY_NT_SOURCE to select the copy-out path. Above that threshold, NT_DEST is intentionally forced. Individual chunks smaller than 3072 bytes still use memcpy().
|
PR size exceeds the 500-added-line guidance (already noted); the AVX2/AVX-512 kernels could plausibly be split, though it is a self-contained rewrite of one routine. Author's call. Test coverage: |
Signed-off-by: Arun Chandran <Arun.Chandran@amd.com>
Signed-off-by: Arun Chandran <Arun.Chandran@amd.com>
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
| { | ||
| if (ucs_arch_get_cpu_vendor() == UCS_CPU_VENDOR_AMD) { | ||
| return ucs_cpu_get_cache_size(UCS_CPU_CACHE_L3) * 9 / 8; | ||
| return ucs_cpu_get_cache_size(UCS_CPU_CACHE_L3) * 3 / 4; |
There was a problem hiding this comment.
maybe consider splitting the PR like:
- code motion, move existing AVX2 to cpu_nt_avx2.inl, extract memcpy erms, hopefully trivial to approve.
- behavioral rework thresholds, erms for nt source
- AVX512 related changes, possibly easier to review as it is new
There was a problem hiding this comment.
Okay, I’ll split this into logical PRs. I’ve opened the first one: #11878
This branch has not been deployed
4 participants
What
Why?
New feature
How?
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
UCX_BUILTIN_MEMCPY_MAX=0
The default UCX_NT_BUFFER_TRANSFER_MIN start threshold is three-quarters of the detected L3 cache size.