Skip to content

Conversation

@Binyang2014
Copy link
Contributor

Fix cross-compiling issue. Add target_compile_options to make sure the compile option is correct

Copy link
Contributor

Copilot AI left a comment

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 fixes ROCm cross-compilation issues by ensuring correct compile options are applied to target architectures. The fix standardizes the GPU architecture variable name and adds explicit compile options for ROCm targets.

Key changes:

  • Renamed CMAKE_HIP_ARCHITECTURES to MSCCLPP_GPU_ARCHS for consistency
  • Added target_compile_options with --offload-arch flags for ROCm builds in both main library and NCCL app

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
CMakeLists.txt Updates GPU architecture variable name and adds compile options for main library
apps/nccl/CMakeLists.txt Adds compile options for NCCL app ROCm builds

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@Binyang2014 Binyang2014 requested a review from seagater October 1, 2025 20:44
Copy link
Contributor

@seagater seagater left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Binyang2014 Binyang2014 merged commit 79e9e61 into main Oct 1, 2025
14 checks passed
@Binyang2014 Binyang2014 deleted the binyli/rocm branch October 1, 2025 21:20
@Binyang2014 Binyang2014 restored the binyli/rocm branch October 1, 2025 21:21
Binyang2014 added a commit that referenced this pull request Oct 3, 2025
Fix cross-compiling issue. Add `target_compile_options` to make sure the
compile option is correct
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.

3 participants

SYSTEM_READY >> ...MS