-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Build issues on AIX for POWER10 and POWER11 #26704
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@KV2773 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this 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 addresses build failures on AIX for POWER10 and POWER11 systems caused by macro redefinition errors. The fix reorders the inclusion of the system header <sys/systemcfg.h> to occur before defining fallback macros, and adds conditional guards to prevent redefining POWER_10 and POWER_10_ANDUP if they're already defined by the system.
Key Changes:
- Move
#include <sys/systemcfg.h>before macro definitions to check system-provided macros first - Add
#if !defined(POWER_10)guard around fallback macro definitions - Apply changes consistently in both runtime code and CMake build configuration
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| onnxruntime/core/mlas/lib/platform.cpp | Adds conditional guards around POWER_10 macro definitions for AIX platform detection, preventing redefinition errors when system headers already provide these macros |
| cmake/onnxruntime_mlas.cmake | Applies the same macro guard pattern in CMake compile checks to ensure consistent build configuration for POWER10 runtime detection on AIX |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #if !defined(POWER_10) | ||
| #define POWER_10 0x40000 | ||
| #define POWER_10_ANDUP (POWER_10) |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The guard only checks if POWER_10 is defined, but POWER_10_ANDUP (used on line 38) might not be defined by the system header. Consider also adding a guard for POWER_10_ANDUP:
#if !defined(POWER_10_ANDUP)
#define POWER_10_ANDUP (POWER_10)
#endifThis ensures POWER_10_ANDUP is always available even if the system header only defines POWER_10.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
Removing spaces. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
/azp run Linux QNN CI Pipeline,Win_TRT_Minimal_CUDA_Test_CI,Windows ARM64 QNN CI Pipeline,Windows GPU Doc Gen CI Pipeline |
|
Azure Pipelines successfully started running 4 pipeline(s). |
Defining POWER_10_ANDUP Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
KV2773
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating the commit as mentioned by the @copilot.
| #if !defined(POWER_10) | ||
| #define POWER_10 0x40000 | ||
| #define POWER_10_ANDUP (POWER_10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
|
/azp run Linux QNN CI Pipeline,Win_TRT_Minimal_CUDA_Test_CI,Windows ARM64 QNN CI Pipeline,Windows GPU Doc Gen CI Pipeline |
|
Azure Pipelines successfully started running 4 pipeline(s). |
| #include <sys/systemcfg.h> | ||
| #if !defined(POWER_10) | ||
| #define POWER_10 0x40000 | ||
| #define POWER_10_ANDUP (POWER_10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe similar POWER_10_ANDUP gaurd here is in the other file ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea here also we can add that line checking for the POWER_10_ANDUP macro.
While building onnxruntime from source for AIX I ran into macro pre-defined errors for the POWER10 and POWER11 machines. This patch resolves the issue.