Skip to content

Make sure FlagCX can be compiled on non-NVIDIA platforms#479

Closed
tengqm wants to merge 1 commit into
mainfrom
fix-compilation-non-nvidia
Closed

Make sure FlagCX can be compiled on non-NVIDIA platforms#479
tengqm wants to merge 1 commit into
mainfrom
fix-compilation-non-nvidia

Conversation

@tengqm

@tengqm tengqm commented May 24, 2026

Copy link
Copy Markdown
Contributor

PR Category

Others

PR Types

Bug Fix

PR Description

When compiling FlagCX on non-NVIDIA platforms, there is an error about the flagcxInnerWindow not defined. This PR fixes the problem.

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 aims to fix a build failure on non-NVIDIA platforms by ensuring flagcxInnerWindow has a concrete definition available when the NVIDIA adaptor headers are not present.

Changes:

  • Add a fallback struct flagcxInnerWindow { int winFlags; } definition in sym_heap.h behind a FLAGCX_INNER_WINDOW_DEFINED macro guard.
  • Define FLAGCX_INNER_WINDOW_DEFINED in nvidia_adaptor.h to prevent the fallback definition from being used when building with the NVIDIA adaptor.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
flagcx/core/include/sym_heap.h Adds a guarded fallback definition of flagcxInnerWindow for non-vendor/non-NVIDIA builds.
flagcx/adaptor/include/nvidia_adaptor.h Defines a macro intended to suppress the fallback flagcxInnerWindow definition when NVIDIA adaptor types are used.
Comments suppressed due to low confidence (1)

flagcx/adaptor/include/nvidia_adaptor.h:32

  • FLAGCX_INNER_WINDOW_DEFINED is defined unconditionally here, but the actual flagcxInnerWindow layout differs from the fallback definition added in sym_heap.h when NCCL_VERSION_CODE > 2.27 (this header places base before winFlags). Any TU that sees the fallback definition (because it included sym_heap.h before this header, or never includes this header) but later receives a vendorBase allocated with the NVIDIA layout will misinterpret winFlags.

Consider making winFlags the first member in the NVIDIA flagcxInnerWindow as well (so generic code can safely access it), and/or guarding the macro/struct definition so the type cannot be defined inconsistently depending on include order.

#define FLAGCX_INNER_WINDOW_DEFINED

#if NCCL_VERSION_CODE > NCCL_VERSION(2, 27, 0)

struct flagcxInnerWindow {
  ncclWindow_t base;
  int winFlags;
};

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +17 to +19
struct flagcxInnerWindow {
int winFlags;
};
@MC952-arch

Copy link
Copy Markdown
Collaborator

Please rebase the code

@MC952-arch MC952-arch added the bug Something isn't working label Jun 1, 2026
@MC952-arch

Copy link
Copy Markdown
Collaborator

Please refer to #488, I've created a new PR to fix this issue

@MC952-arch MC952-arch closed this Jun 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants