Skip to content

[c10d][xccl2] Port the XCCL work object, work queue and bootstrap - #7

Open
frost-intel wants to merge 1 commit into
xccl2/02-apifrom
xccl2/03-work
Open

frost-intel wants to merge 1 commit into
xccl2/02-apifrom
xccl2/03-work

Conversation

@frost-intel

@frost-intel frost-intel commented Aug 25, 2026

Copy link
Copy Markdown
Owner

Stack (bottom → top)

PR Branch
#5 xccl2/01-foundation
#6 xccl2/02-api
#7 xccl2/03-work
#8 xccl2/04-engine-decl
#9 xccl2/05-engine-impl
#10 xccl2/06-backend-surface
#11 xccl2/07-build-wiring
#12 xccl2/08-tests
#3 xccl2/09-integration-tests

Each PR is exactly one commit and targets the one below it. Review bottom-up.


Ports comms/torchcomms/xccl/TorchWorkXCCL.{hpp,cpp},
TorchWorkXCCLQueue.cpp and TorchCommXCCLBootstrap.{hpp,cpp} to
torch/csrc/distributed/c10d/xccl2/ as WorkXCCL.{hpp,cpp},
WorkXCCLQueue.cpp and XCCLBootstrap.{hpp,cpp}. Still not referenced by
anything; build wiring lands later in the stack.

Behaviour is kept identical to torchcomms. Completion is still tracked with a
pair of XPU events, and the timeout is still wall-clock measured from the
moment the start event is observed complete, so time spent queued behind
earlier collectives is not charged against a work's timeout.

Structural changes, all mirroring how nccl2 ported the NCCL equivalents:

  • WorkXCCL derives from c10d::Work rather than torchcomms::TorchWork, so the
    result()/getFuture() handling that the torchcomms BackendWrapper used to
    supply is folded into the work itself, and the wait pre/post hooks (a
    TorchWork concept) are dropped.
  • The back-pointer to the owning backend is dropped entirely rather than
    ported. torchcomms holds an owning shared_ptr, which c10d
    cannot mirror because backends are owned by intrusive_ptr, but demoting it
    to a raw pointer would be unsound: a work can outlive its backend. Python
    may still hold the handle returned by an async collective when
    destroy_process_group() runs, and ~WorkXCCL returns its events to the
    backend's pool, so the destructor would lock a destroyed mutex inside freed
    memory. Instead the work borrows nothing: it snapshots the name, rank, size
    and device it needs for logging and stream lookup into an XcclCommInfo at
    construction, and shares ownership of the event pool, which is hoisted out
    of the backend into a shared_ptr so it outlives it. An
    owning intrusive_ptr back to the backend was rejected because the backend
    reaches the work through its queue, and the resulting cycle would leak a
    communicator whenever the user never calls destroy_process_group().
  • torchcomms' XpuApi indirection is not ported. It exists so torchcomms can
    mock the driver in its unit tests; in-tree, at::xpu::XPUEvent wraps the same
    sycl::event, so eventRecord/eventQuery/streamWaitEvent become
    record()/query()/block(). Because XPUEvent reports errors by throwing rather
    than by returning a status, the XPU_ERROR_NOT_READY / XPU_ERROR_UNSUPPORTED
    checks become try/catch, matching nccl2's handling of CUDAEvent.
  • Completed work is parked on a completed_work_queue_ and released by the next
    caller-thread enqueueWork() instead of being dropped inline in checkStatus().
    This is nccl2's fix, taken deliberately: dropping the last reference to an
    at::Tensor from the watchdog thread runs tensor destruction off a thread
    c10d does not control.
  • XCCLBootstrap takes rank/size/store from the ProcessGroup and keys the
    unique ID off a PrefixStore as "xccl_storekey_". torchcomms
    additionally supported building its own TCPStore from MASTER_ADDR/MASTER_PORT
    when no Store was supplied; in c10d the Store is always supplied, so that
    path -- along with TORCHCOMM_XCCL_BOOTSTRAP_UNIQUEID_EXCHANGE_METHOD, the
    internal-store teardown barrier and its XPU barrier buffer -- is unreachable
    and is not ported. Same reduction nccl2 made.

populateXcclConfigFromHints keeps torchcomms' targeted warning for hints that
are NCCL-only (trafficClass, CTAPolicy, nvlsCTAs, ...) so that a hint dict
carried over from nccl2 reports which keys oneCCL has no equivalent for. The
strdup() for config.netName is kept as-is: oneCCL's ownership of that pointer
past onecclCommInitRankConfig is undocumented, so this deliberately leaks
rather than risk a dangling pointer.

Typechecked against the real torch_xpu compile flags; all three TUs report
only the build tree's pre-existing stale-TensorBody.h error, same as the
untouched control TUs.

Ports comms/torchcomms/xccl/TorchWorkXCCL.{hpp,cpp},
TorchWorkXCCLQueue.cpp and TorchCommXCCLBootstrap.{hpp,cpp} to
torch/csrc/distributed/c10d/xccl2/ as WorkXCCL.{hpp,cpp},
WorkXCCLQueue.cpp and XCCLBootstrap.{hpp,cpp}. Still not referenced by
anything; build wiring lands later in the stack.

Behaviour is kept identical to torchcomms. Completion is still tracked with a
pair of XPU events, and the timeout is still wall-clock measured from the
moment the *start* event is observed complete, so time spent queued behind
earlier collectives is not charged against a work's timeout.

Structural changes, all mirroring how nccl2 ported the NCCL equivalents:

- WorkXCCL derives from c10d::Work rather than torchcomms::TorchWork, so the
  result()/getFuture() handling that the torchcomms BackendWrapper used to
  supply is folded into the work itself, and the wait pre/post hooks (a
  TorchWork concept) are dropped.
- The back-pointer to the owning backend is dropped entirely rather than
  ported. torchcomms holds an owning shared_ptr<TorchCommXCCL>, which c10d
  cannot mirror because backends are owned by intrusive_ptr, but demoting it
  to a raw pointer would be unsound: a work can outlive its backend. Python
  may still hold the handle returned by an async collective when
  destroy_process_group() runs, and ~WorkXCCL returns its events to the
  backend's pool, so the destructor would lock a destroyed mutex inside freed
  memory. Instead the work borrows nothing: it snapshots the name, rank, size
  and device it needs for logging and stream lookup into an XcclCommInfo at
  construction, and shares ownership of the event pool, which is hoisted out
  of the backend into a shared_ptr<XcclEventPool> so it outlives it. An
  owning intrusive_ptr back to the backend was rejected because the backend
  reaches the work through its queue, and the resulting cycle would leak a
  communicator whenever the user never calls destroy_process_group().
- torchcomms' XpuApi indirection is not ported. It exists so torchcomms can
  mock the driver in its unit tests; in-tree, at::xpu::XPUEvent wraps the same
  sycl::event, so eventRecord/eventQuery/streamWaitEvent become
  record()/query()/block(). Because XPUEvent reports errors by throwing rather
  than by returning a status, the XPU_ERROR_NOT_READY / XPU_ERROR_UNSUPPORTED
  checks become try/catch, matching nccl2's handling of CUDAEvent.
- Completed work is parked on a completed_work_queue_ and released by the next
  caller-thread enqueueWork() instead of being dropped inline in checkStatus().
  This is nccl2's fix, taken deliberately: dropping the last reference to an
  at::Tensor from the watchdog thread runs tensor destruction off a thread
  c10d does not control.
- XCCLBootstrap takes rank/size/store from the ProcessGroup and keys the
  unique ID off a PrefixStore as "xccl_storekey_<generation>". torchcomms
  additionally supported building its own TCPStore from MASTER_ADDR/MASTER_PORT
  when no Store was supplied; in c10d the Store is always supplied, so that
  path -- along with TORCHCOMM_XCCL_BOOTSTRAP_UNIQUEID_EXCHANGE_METHOD, the
  internal-store teardown barrier and its XPU barrier buffer -- is unreachable
  and is not ported. Same reduction nccl2 made.

populateXcclConfigFromHints keeps torchcomms' targeted warning for hints that
are NCCL-only (trafficClass, CTAPolicy, nvlsCTAs, ...) so that a hint dict
carried over from nccl2 reports which keys oneCCL has no equivalent for. The
strdup() for config.netName is kept as-is: oneCCL's ownership of that pointer
past onecclCommInitRankConfig is undocumented, so this deliberately leaks
rather than risk a dangling pointer.

Typechecked against the real torch_xpu compile flags; all three TUs report
only the build tree's pre-existing stale-TensorBody.h error, same as the
untouched control TUs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant