[UIL] Add Unified IR support - #539
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a transport-transparent “Unified One-Sided IR” device API layer (flagcxDev*) that auto-dispatches between intra-node P2P (IPC/NVLink) and inter-node Net/FIFO paths, along with new unit-test suites and supporting runtime changes (signal/counter slot types, IPC peer pointers for signal buffers, and updated trigger bit packing).
Changes:
- Add new unified IR C ABI (
bindings/ir/flagcx_device_unified_ir*.h) implementing put/get/putValue/signal/wait/read/flush/reset/barrier with P2P-vs-Net dispatch. - Extend device comm internals and comm traits to support P2P signal delivery via IPC-mapped peer signal-buffer pointers.
- Add new unified intra/inter test binaries and wire them into Makefile + CI.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unittest/shmem_adaptor/test_nvshmem_adaptor.cu | Update signal/counter slot typedef usage in shmem adaptor tests. |
| test/unittest/device_api/test_device_ir_unified.cpp | Add legacy 12-combination unified one-sided test driver. |
| test/unittest/device_api/test_device_ir_unified_intra.cpp | Add unified INTRA+WORLD suite test driver. |
| test/unittest/device_api/test_device_ir_unified_inter.cpp | Add unified INTER+WORLD suite test driver. |
| test/unittest/device_api/Makefile | Build/run targets for the new unified test binaries. |
| test/make.inc | Relax DEVICE_PLATFORM requirement for non-build goals. |
| test/kernel/nvidia/nvshmem_adaptor_kernel.cu | Update signal/counter slot typedef usage in NVSHMEM adaptor kernels. |
| test/kernel/nvidia/device_api.cu | Update signal slot typedef usage in device API kernels. |
| test/kernel/include/device_ir.h | Export unified test kernel launch APIs; include new enums header. |
| test/kernel/du/device_ir.cu | Update signal/counter slot typedef usage in DU device IR kernels. |
| test/kernel/du/device_api.cu | Update signal slot typedef usage in DU device API kernels. |
| flagcx/include/flagcx_kernel_core.h | Expand signalIdx bit-width in trigger packing (8→12 bits). |
| flagcx/core/proxy.cc | Reduce noisy INFO/WARN logs; add proxy loop exit summary log. |
| flagcx/adaptor/include/device_api/nvshmem_comm_traits.h | Add P2P signal support hooks; update slot typedefs. |
| flagcx/adaptor/include/device_api/nccl_comm_traits.h | Add P2P signal support hooks (disabled); update slot typedefs. |
| flagcx/adaptor/include/device_api/flagcx_device_internal.h | Add signalPeerPtrs + IPC slot tracking to DevComm internal. |
| flagcx/adaptor/include/device_api/flagcx_device_enums.h | Introduce unified slot typedefs (flagcxDevSignal_t, etc.). |
| flagcx/adaptor/include/device_api/flagcx_device_core.h | Update Net APIs to use new slot typedefs. |
| flagcx/adaptor/include/device_api/default_comm_traits.h | Add intraSize + signalPeerPtrs; fix peer pointer indexing; team-rank→world mapping. |
| flagcx/adaptor/include/device_api/comm_traits.h | Move to unified slot typedefs in action structs. |
| flagcx/adaptor/flagcx_device.cc | Initialize new signal IPC slot field. |
| flagcx/adaptor/device_api/default_dev_api_backend.cc | Allocate one-sided buffers more generally; add IPC setup/cleanup for signalPeerPtrs. |
| bindings/ir/public_symbols.txt | Export new unified IR symbols. |
| bindings/ir/flagcx_device_wrapper.h | Update wrapper signatures to use new slot typedefs. |
| bindings/ir/flagcx_device_wrapper_impl.h | Update wrapper impl signatures to use new slot typedefs. |
| bindings/ir/flagcx_device_unified_ir.h | New: unified transport-transparent device IR public header. |
| bindings/ir/flagcx_device_unified_ir_impl.h | New: unified IR implementation (P2P dispatch + Net fallback). |
| bindings/ir/flagcx_device_scalar_ir.h | Deprecate Net-specific APIs in favor of unified APIs; update slot typedefs. |
| bindings/ir/flagcx_device_scalar_ir_impl.h | Update scalar IR impl to use new slot typedefs; include unified IR impl. |
| .github/scripts/ci/run_unit_test.sh | Run unified intra/inter test binaries in CI. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
854d43f to
d12b7bf
Compare
8a1d88d to
63bcf94
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated no new comments.
Suppressed comments (7)
test/unittest/device_api/test_device_ir_unified.cpp:135
- hostSend/hostRecv are over-allocated by an extra factor of 12: bufSize is already maxBytes*12 (bytes), so multiplying by 12 again can allocate >1GB at default sizes and OOM CI runs. These buffers only need to cover the device buffer size.
// Host scratch - allocate 12× buffers for 12 combinations
float *hostSend = new float[bufSize * 12 / sizeof(float)];
float *hostRecv = new float[bufSize * 12 / sizeof(float)];
test/unittest/device_api/test_device_ir_unified_intra.cpp:132
- hostSend/hostRecv are over-allocated by an extra factor of 8: bufSize is already maxBytes*8 (bytes), so multiplying by 8 again needlessly inflates host memory usage and may OOM on large -e values.
test/unittest/device_api/test_device_ir_unified_inter.cpp:136 - hostSend/hostRecv are over-allocated by an extra factor of 8: bufSize is already maxBytes*8 (bytes), so multiplying by 8 again needlessly inflates host memory usage and may OOM on large -e values.
// Host scratch - allocate 8× buffers for S18's 8 combinations
float *hostSend = new float[bufSize * 8 / sizeof(float)];
float *hostRecv = new float[bufSize * 8 / sizeof(float)];
test/unittest/device_api/test_device_ir_unified_intra.cpp:651
- sendBuff/recvBuff are allocated with a non-default allocator (memAllocator) but freed using the default allocator (flagcxMemCCL). If FLAGCX_COMM_TRAITS_SHMEM is enabled this will free with the wrong backend. Pass memAllocator to flagcxMemFree, matching the allocation site.
test/unittest/device_api/test_device_ir_unified_inter.cpp:685 - sendBuff/recvBuff are allocated with a non-default allocator (memAllocator) but freed using the default allocator (flagcxMemCCL). If FLAGCX_COMM_TRAITS_SHMEM is enabled this will free with the wrong backend. Pass memAllocator to flagcxMemFree, matching the allocation site.
FLAGCXCHECK(flagcxCommWindowDeregister(comm, sendWin, memAllocator));
FLAGCXCHECK(flagcxCommWindowDeregister(comm, recvWin, memAllocator));
FLAGCXCHECK(flagcxMemFree(sendBuff));
FLAGCXCHECK(flagcxMemFree(recvBuff));
test/unittest/device_api/Makefile:126
- run-mpi-unified-intra omits $(HETERO_ENV), but the unified intra test itself documents FLAGCX_USE_HETERO_COMM=1 as a requirement (for DevComm). As written, local
make run-mpican fail while CI passes because CI injects env separately.
# Unified Intra suite: INTRA + WORLD teams (single-node)
run-mpi-unified-intra: $(BINDIR)/test_device_ir_unified_intra
@echo "Running unified intra suite tests (INTRA + WORLD, 8-combo)..."
mpirun -np 8 --allow-run-as-root $(BINDIR)/test_device_ir_unified_intra
bindings/ir/flagcx_device_wrapper.h:328
- The Category 11 header still claims “One-Sided put (16)”, but only 6 put variants are declared in this section now. Please update the section header comment so IR consumers don’t assume the removed combinations are still available.
* @param coop Pointer to flagcxCoopAny struct.
* @param remoteSignal Remote signal slot (R variants).
* @param remoteValue Value to add to remote signal (RSigAdd).
* @param localCounter Local counter slot (LCtrInc).
* ================================================================ */
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 40 changed files in this pull request and generated no new comments.
Suppressed comments (3)
test/unittest/device_api/test_device_ir_unified.cpp:14
- This new 670-line test file is not wired into the build: the
Makefileonly buildstest_device_ir_unified_intraandtest_device_ir_unified_inter, and no CI script referencestest_device_ir_unified. As submitted it will never be compiled or run, so it can silently rot. Additionally, this header docstring is stale — it listsS16: Unified Put … S22: Team-resolution, but the actual blocks below are renumbered (S16–S18 are barriers, S19 is team-resolution, S20 Put, S21 Get, S22 Signal, S23 Put+Signal+Wait). Please either add a build/run target for this file (and fix the docstring) or remove it.
test/unittest/device_api/test_device_ir_unified.cpp:205 - On a single node (
nNodes == 1) this block prints "SKIP (single-node)" but still executesallPass &= s17Passwiths17Pass = (hostRes == 1). IflaunchKernelDevBarrierInterSdoes not set the result to 1 when there are no inter-node peers, a "skipped" inter-node barrier will fail the whole suite. TheallPassaccumulation should be gated onnNodes > 1so a genuinely skipped case cannot flip the result.
bindings/ir/flagcx_device_scalar_ir_impl.h:529 - This label is incorrect for
flagcxDevNetPutS_LCtrInc, which issues aflagcxDevNet_None{}remote action plus a localflagcxDevNet_CounterInc. It should read(None, CtrInc)(matching the declaration inflagcx_device_scalar_ir.h);(SigInc, CtrInc)is the correct label for the followingflagcxDevNetPutS_RSigInc_LCtrIncvariant.
/* (SigInc, CtrInc) */
PR Category
UIL
PR Types
New Features
PR Description
This PR introduces a transport-transparent “Unified One-Sided IR” device API layer (flagcxDev*) that auto-dispatches between intra-node P2P (IPC/NVLink) and inter-node Net/FIFO paths, along with new unit-test suites and supporting runtime changes (signal/counter slot types, IPC peer pointers for signal buffers, and updated trigger bit packing).