Skip to content

Fix CPUInfo logging #15661

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.

Sign up for GitHub

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

Closed
tianleiwu wants to merge 4 commits into from
Closed

Fix CPUInfo logging #15661

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
549085d
Fix CPUInfo logging
tianleiwu Apr 24, 2023
0a7f55c
fix build
tianleiwu Apr 24, 2023
61bdccb
constexpr
tianleiwu Apr 25, 2023
d0af332
fix const char*
tianleiwu Apr 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion onnxruntime/core/common/cpuid_info.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include "core/common/cpuid_info.h"
#include "core/common/logging/logging.h"
#include "core/common/logging/severity.h"
#include <iostream>

#ifdef __linux__

Expand Down Expand Up @@ -132,7 +133,13 @@ void CPUIDInfo::ArmLinuxInit() {
#ifdef CPUINFO_SUPPORTED
pytorch_cpuinfo_init_ = cpuinfo_initialize();
Copy link
Contributor

@snnn snnn Apr 25, 2023

Choose a reason for hiding this comment

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

You may also let Environment::Initialize() initialize CPUIDInfo. Then we can have a more user-friendly exception handling, and also this class would not need to use logging since we can return the full error message by using an onnxruntime::Status. And even if it needs to, it would be fine because we can be sure logging is already initialized when Environment::Initialize() is called.

Copy link
Contributor

@chenfucn chenfucn Apr 25, 2023

Choose a reason for hiding this comment

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

You may also let Environment::Initialize() initialize CPUIDInfo. Then we can have a more user-friendly exception handling, and also this class would not need to use logging since we can return the full error message by using an onnxruntime::Status. And even if it needs to, it would be fine because we can be sure logging is already initialized when Environment::Initialize() is called.

That change may cause trouble. MLAS::Platform depends on CPUIDInfo. Currently CPUIDInfo must be initialized before the Platform static initializer is called.

The proposed change, if done properly, would be a very big endeavor. First we need to add an MLAS init interface to be called by Environment::Initialize(). Second we need to change all other MLAS APIs to return some error status if called before MLAS init. And third MLAS is not the only one using static initialization in ORT, we need to find all of them and figure out dependency relationships among them, break possible circular dependencies.

I am not saying don't do it. I am suggesting that such action needs planning and coordination between multiple teams.

Copy link
Contributor

@snnn snnn Apr 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Since MLAS::Platform is initialized after CPUIDInfo, can we start to change that part? I don't see any blocker there. ORT usually does not use static initialization, and we have a tool to find out all dynamic initializers. If there is no dependency between these dynamic initializers, it would be fine. For example, if the constructor of CPUIDInfo doesn't use global logger, and the CPUINFO library also doesn't have any dynamic initializer, this code would work fine.

Copy link
Contributor

@chenfucn chenfucn May 3, 2023

Choose a reason for hiding this comment

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

How? platform needs CPUIDInfo to know which cpu it is running on, which in turn let MLAS pick the best assembly kernels to achieve high performance.

if (!pytorch_cpuinfo_init_) {
LOGS_DEFAULT(WARNING) << "Failed to init pytorch cpuinfo library, may cause CPU EP performance degradation due to undetected CPU features.";
constexpr const char* message = "Failed to init pytorch cpuinfo library, may cause CPU EP performance degradation due to undetected CPU features.";
if (logging::LoggingManager::HasDefaultLogger()) {
LOGS_DEFAULT(WARNING) << message;
} else {
std::cerr << message << std::endl;
}

return;
}
#else
Expand Down