Skip to content

Enable collective tests#974

Open
mmakevic-amd wants to merge 5 commits into
mainfrom
mmakevic/rocm_enable_collective_tests_rocm_main
Open

Enable collective tests#974
mmakevic-amd wants to merge 5 commits into
mainfrom
mmakevic/rocm_enable_collective_tests_rocm_main

Conversation

@mmakevic-amd

@mmakevic-amd mmakevic-amd commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Motivation

This PR enables:

  • //xla/backends/gpu/collectives:gpu_cliques_test,
  • //xla/backends/gpu/collectives:gpu_collectives_test, and
  • //xla/backends/gpu/collectives:loopback_collectives_test.

It also extracts ToNcclCount, ToNcclDataType and ToNcclReduction from xla/backends/gpu/collectives/rccl_communicator.cc to the newly added xla/backends/gpu/collectives/rccl_types.cc and adds xla/backends/gpu/collectives/rccl_types_test.cc. This mirrors nccl_types.cc and nccl_types_test.cc.

Technical Details

  1. Switched all enabled tests from xla_cc_test to xla_test target
  2. Generalized enabled tests to use the provided platform instead of hardcoded CUDA
  3. As mentioned above, separated rccl_types in a separate file and added the appropriate test, mirroring CUDA implementation.

Test Plan

/

Test Result

There are five failing subtests in gpu_collectives_test which will be addressed later. Other tests are passing.

[  FAILED  ] GpuCollectivesTest.CreateRegisteredMemory
[  FAILED  ] GpuCollectivesTest.CreateWithMultipleIds
[  FAILED  ] GpuCollectivesTest.SplitCommunicators
[  FAILED  ] GpuCollectivesTest.CreateSymmetricMemory
[  FAILED  ] GpuCollectivesTest.PutAndWaitSignal

Submission Checklist

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enables previously-disabled GPU collectives tests across GPU backends (CUDA/ROCm), and refactors RCCL-specific NCCL-type conversion helpers into a standalone rccl_types library with a new unit test, mirroring the existing NCCL implementation.

Changes:

  • Enable gpu_cliques_test, gpu_collectives_test, and loopback_collectives_test by converting targets from xla_cc_test to xla_test and generalizing platform selection away from hardcoded "CUDA".
  • Extract ToNcclCount, ToNcclDataType, and ToNcclReduction from rccl_communicator.cc into new rccl_types.{h,cc}.
  • Add rccl_types_test.cc to validate RCCL type conversions.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
xla/backends/gpu/collectives/rccl_types.h Adds RCCL type conversion API declarations.
xla/backends/gpu/collectives/rccl_types.cc Implements RCCL type conversion helpers extracted from communicator.
xla/backends/gpu/collectives/rccl_types_test.cc Adds unit tests for RCCL type conversions.
xla/backends/gpu/collectives/rccl_communicator.cc Switches communicator code to use extracted type conversion helpers.
xla/backends/gpu/collectives/loopback_collectives_test.cc Generalizes platform selection via PlatformUtil instead of hardcoded CUDA.
xla/backends/gpu/collectives/gpu_collectives_test.cc Generalizes platform selection and GPU capability checks across backends.
xla/backends/gpu/collectives/gpu_cliques_test.cc Generalizes platform selection via PlatformUtil instead of hardcoded CUDA.
xla/backends/gpu/collectives/BUILD Enables tests via xla_test, adds rccl_types targets, and adjusts deps/tags per backend.

Comment thread xla/backends/gpu/collectives/rccl_types.cc Outdated
Comment thread xla/backends/gpu/collectives/gpu_cliques_test.cc
Comment thread xla/backends/gpu/collectives/gpu_cliques_test.cc
Comment thread xla/backends/gpu/collectives/loopback_collectives_test.cc
Comment on lines +139 to +142
auto name = absl::AsciiStrToUpper(
xla::PlatformUtil::CanonicalPlatformName("gpu").value());
ASSERT_OK_AND_ASSIGN(se::Platform * platform,
se::PlatformManager::PlatformWithName("CUDA"));
se::PlatformManager::PlatformWithName(name));

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Comment thread xla/backends/gpu/collectives/gpu_collectives_test.cc
Comment thread xla/backends/gpu/collectives/rccl_types_test.cc
Comment thread xla/backends/gpu/collectives/BUILD
Comment thread xla/backends/gpu/collectives/BUILD
Comment thread xla/backends/gpu/collectives/BUILD Outdated
@hsharsha

Copy link
Copy Markdown
Collaborator

Should we upstream it to openxla/main?

@mmakevic-amd

Copy link
Copy Markdown
Collaborator Author

Should we upstream it to openxla/main?

I discussed this with @i-chaochen in https://github.com/ROCm/xla-internal/issues/52#issuecomment-4752364931. Since some of these tests are mGPU, and we won't be enabling them in CI for now, he proposed fixing the failing subtests before upstreaming.

@hsharsha

Copy link
Copy Markdown
Collaborator

I mean the fix, We can enable mgpu tests as nightly

@i-chaochen i-chaochen added the claude-review Request a Claude AI code review for this PR label Jun 22, 2026
Comment thread xla/backends/gpu/collectives/gpu_collectives_test.cc Outdated
Comment on lines +37 to +43
switch (dtype) {
case F8E5M2:
return rocm_cc.has_ocp_fp8_support() ? ncclFloat8e5m2 : ncclInt8;
case F8E4M3FN:
return rocm_cc.has_ocp_fp8_support() ? ncclFloat8e4m3 : ncclInt8;
case S8:
case F8E5M2FNUZ:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug: FP8 reduction silently produces wrong results — The NCCL counterpart uses (cc.IsAtLeastHopper() || is_reduction_op) ? ncclFloat8e5m2 : ncclInt8, deliberately letting NCCL throw errors for FP8 reductions on unsupported hardware. Here, the logic is has_ocp_fp8_support() ? ncclFloat8e5m2 : ncclInt8, ignoring the is_reduction_op flag entirely.

On non-OCP ROCm hardware, FP8 reductions will silently be mapped to ncclInt8, which would produce incorrect results rather than a clean error. This should either mirror the NCCL behavior (using has_ocp_fp8_support() || is_reduction_op) or include a comment explaining why the divergence is intentional.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mfrancepillois, I see you're the one who worked on fp8 support in openxla@bd94197. Could you let me know if this is a legitimate concern? Are reductions supported?

@i-chaochen i-chaochen Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, reduction Ops and all fp8 Ops are supported in rccl since rocm6.4.... https://github.com/ROCm/frameworks-internal/issues/15398

Comment thread xla/backends/gpu/collectives/rccl_types.cc Outdated
Comment thread xla/backends/gpu/collectives/gpu_collectives_test.cc
Comment thread xla/backends/gpu/collectives/gpu_cliques_test.cc
Comment thread xla/backends/gpu/collectives/BUILD
@claude

claude Bot commented Jun 22, 2026

Copy link
Copy Markdown

Review Summary

This PR enables three ROCm collective tests by converting them to xla_test, generalizing hardcoded "CUDA" platform names to runtime-resolved values, and extracting ToNcclCount/ToNcclDataType/ToNcclReduction into new rccl_types.{h,cc} files mirroring the existing nccl_types structure.

Key finding: The FP8 type mapping in rccl_types.cc diverges from nccl_types.cc by ignoring the is_reduction_op flag — on non-OCP ROCm hardware, FP8 reductions would silently map to ncclInt8 producing incorrect results instead of a clean error.

6 inline comments posted covering: FP8 correctness, fragile transitive includes, unchecked .value() repetition, missing ROCm skip conditions, error construction style, and a stale BUILD dep.

🤖 Generated with Claude Code

@github-actions github-actions Bot removed the claude-review Request a Claude AI code review for this PR label Jun 22, 2026
@mmakevic-amd

Copy link
Copy Markdown
Collaborator Author

I mean the fix, We can enable mgpu tests as nightly

@hsharsha, @i-chaochen, here is the draft PR for upstream openxla#44641

The falling subtests in //xla/backends/gpu/collectives:gpu_collectives_test won't affect our CI atm since it is a multi-gpu test. I will look into them once this is merged.

Let me know if it looks good to you, so I can open a PR

@mmakevic-amd mmakevic-amd force-pushed the mmakevic/rocm_enable_collective_tests_rocm_main branch from 41617d0 to dab8b9a Compare June 24, 2026 10:12
@mmakevic-amd mmakevic-amd force-pushed the mmakevic/rocm_enable_collective_tests_rocm_main branch 2 times, most recently from 98b6ec7 to e67e127 Compare June 24, 2026 10:45
@mmakevic-amd mmakevic-amd force-pushed the mmakevic/rocm_enable_collective_tests_rocm_main branch from e67e127 to bbe8f88 Compare June 24, 2026 12:06
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.

4 participants