-
Notifications
You must be signed in to change notification settings - Fork 25
Add loglevels to aiebu #178
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-ge
Are you sure you want to change the base?
Conversation
Signed-off-by: Donwalkar <[email protected]>
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.
clang-tidy made some suggestions
| }; | ||
|
|
||
| // Global log level (default: errors and warnings) | ||
| inline log_level g_log_level = log_level::warn; |
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.
warning: variable 'g_log_level' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
inline log_level g_log_level = log_level::warn;
^| } // namespace aiebu | ||
|
|
||
| // Logging macros | ||
| #define LOG_ERROR(msg) \ |
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.
warning: function-like macro 'LOG_ERROR' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]
#define LOG_ERROR(msg) \
^| // Logging macros | ||
| #define LOG_ERROR(msg) \ | ||
| do { \ | ||
| std::cerr << "[ERROR] " << msg << std::endl; \ |
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.
warning: macro argument should be enclosed in parentheses [bugprone-macro-parentheses]
| std::cerr << "[ERROR] " << msg << std::endl; \ | |
| std::cerr << "[ERROR] " << (msg) << std::endl; \ |
| std::cerr << "[ERROR] " << msg << std::endl; \ | ||
| } while (0) | ||
|
|
||
| #define LOG_WARN(msg) \ |
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.
warning: function-like macro 'LOG_WARN' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]
#define LOG_WARN(msg) \
^| #define LOG_WARN(msg) \ | ||
| do { \ | ||
| if (aiebu::g_log_level >= aiebu::log_level::warn) { \ | ||
| std::cout << "[WARN] " << msg << std::endl; \ |
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.
warning: macro argument should be enclosed in parentheses [bugprone-macro-parentheses]
| std::cout << "[WARN] " << msg << std::endl; \ | |
| std::cout << "[WARN] " << (msg) << std::endl; \ |
| m_parserptr->set_nummem(to_uinteger<uint32_t>(match[3])); | ||
| std::cout << "Core count: " << match[1] << std::endl; | ||
| std::cout << "Memory size: " << match[3] << std::endl; | ||
| LOG_INFO("Core count: " << match[1]); |
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.
warning: avoid do-while loops [cppcoreguidelines-avoid-do-while]
LOG_INFO("Core count: " << match[1]);
^Additional context
src/cpp/common/logger.h:58: expanded from macro 'LOG_INFO'
do { \
^| std::cout << "Core count: " << match[1] << std::endl; | ||
| std::cout << "Memory size: " << match[3] << std::endl; | ||
| LOG_INFO("Core count: " << match[1]); | ||
| LOG_INFO("Memory size: " << match[3]); |
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.
warning: avoid do-while loops [cppcoreguidelines-avoid-do-while]
LOG_INFO("Memory size: " << match[3]);
^Additional context
src/cpp/common/logger.h:58: expanded from macro 'LOG_INFO'
do { \
^| return false; | ||
| } | ||
| // std::cout << "Reading file:" << filename << std::endl; | ||
| LOG_INFO("Reading file:" << filename); |
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.
warning: avoid do-while loops [cppcoreguidelines-avoid-do-while]
LOG_INFO("Reading file:" << filename);
^Additional context
src/cpp/common/logger.h:58: expanded from macro 'LOG_INFO'
do { \
^| } | ||
|
|
||
| // std::cout << "Reading file:" << filename << std::endl; | ||
| LOG_INFO("Reading file:" << filename); |
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.
warning: avoid do-while loops [cppcoreguidelines-avoid-do-while]
LOG_INFO("Reading file:" << filename);
^Additional context
src/cpp/common/logger.h:58: expanded from macro 'LOG_INFO'
do { \
^| { | ||
| auto e = as.get_elf(); | ||
| std::cout << "elf size:" << e.size() << "\n"; | ||
| LOG_INFO("elf size:" << e.size()); |
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.
warning: avoid do-while loops [cppcoreguidelines-avoid-do-while]
LOG_INFO("elf size:" << e.size());
^Additional context
src/cpp/common/logger.h:58: expanded from macro 'LOG_INFO'
do { \
^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 adds a structured logging system to the aiebu-asm utility to improve performance by reducing unnecessary console output. The implementation introduces four log levels (error, warn, info, debug) with warn as the default, replacing direct std::cout calls with appropriate logging macros throughout the codebase.
Key changes:
- Created a new logging framework with configurable log levels (error, warn, info, debug)
- Added
--log-levelCLI option to both aie4 and config targets - Replaced console output statements with appropriate LOG_* macros based on message severity
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/cpp/common/logger.h | New header defining log levels, global log level management, and logging macros |
| src/cpp/utils/target/target.cpp | Added log-level option parsing and validation for both aie4 and config targets |
| src/cpp/utils/target/target.h | Replaced std::cout with LOG_INFO for ELF size output |
| src/cpp/preprocessor/asm/asm_parser.cpp | Converted std::cout statements to appropriate LOG_INFO and LOG_WARN calls |
| src/cpp/preprocessor/aie2ps/aie2ps_preprocessor.h | Refactored flag processing to enable verbose logging before parsing |
| src/cpp/preprocessor/aie2/aie2_blob_preprocessor_input.h | Added verbose flag handling for blob preprocessor |
| src/cpp/encoder/aie2ps/aie2ps_encoder.cpp | Conditionalized report summary output based on log level |
| src/cpp/elf/elfwriter.cpp | Replaced std::cout with LOG_INFO for UID output |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (result.count("log-level")) { | ||
| log_level_str = result["log-level"].as<decltype(log_level_str)>(); | ||
| if (log_level_str == "error") | ||
| aiebu::set_log_level(aiebu::log_level::error); | ||
| else if (log_level_str == "warn") | ||
| aiebu::set_log_level(aiebu::log_level::warn); | ||
| else if (log_level_str == "info") | ||
| aiebu::set_log_level(aiebu::log_level::info); | ||
| else if (log_level_str == "debug") | ||
| aiebu::set_log_level(aiebu::log_level::debug); | ||
| else { | ||
| auto errMsg = boost::format("Invalid log level: %s. Valid options: error, warn, info, debug\n") % log_level_str; | ||
| throw std::runtime_error(errMsg.str()); | ||
| } | ||
| } |
Copilot
AI
Nov 25, 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 log level validation logic is duplicated in both the aie4 target (lines 385-399) and config target (lines 475-489). Consider extracting this validation into a reusable helper function to reduce code duplication and improve maintainability.
| auto flags = tinput->get_flags(); | ||
| uint32_t optimize = 0; | ||
| asm_dump_flag debug_flag = asm_dump_flag::text; | ||
| for (const auto& flag: flags) | ||
| { | ||
| if (flag == disable_dump_map) | ||
| toutput->set_debug(asm_dump_flag::disable); | ||
| debug_flag = asm_dump_flag::disable; | ||
| else if (flag == full_dump_map) | ||
| toutput->set_debug(asm_dump_flag::full); | ||
| debug_flag = asm_dump_flag::full; | ||
| else if (flag == verbose_asm) | ||
| enable_verbose_logging(); | ||
| else if (flag.find(prefix) == 0) | ||
| optimize = std::stoi(flag.substr(prefix.size())); | ||
| else | ||
| std::cout << "Invalid flag: " << flag << ", ignored !!!" << std::endl; | ||
| } |
Copilot
AI
Nov 25, 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 error message for invalid flags (line 65) should use LOG_WARN instead of std::cout for consistency with the new logging system.
| else if (lib == "verbose") | ||
| enable_verbose_logging(); | ||
| else | ||
| std::cout << "Invalid flag: " << lib << ", ignored !!!" << std::endl; |
Copilot
AI
Nov 25, 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 error message for invalid flags should use LOG_WARN instead of std::cout for consistency with the new logging system.
| std::cout << "Invalid flag: " << lib << ", ignored !!!" << std::endl; | |
| LOG_WARN("Invalid flag: %s, ignored !!!", lib.c_str()); |
| ("asm,c", "ASM File", cxxopts::value<decltype(input_file)>()) | ||
| ("j,json", "control packet Patching json file", cxxopts::value<decltype(external_buffers_file)>()) | ||
| ("L,libpath", "libs path", cxxopts::value<decltype(libpaths)>()) | ||
| ("f,flag", "flags", cxxopts::value<decltype(flags)>()) |
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.
missed adding for aie2ps, flag & log_level
| ("c,controlcode", "TXN control code binary or ASM file", cxxopts::value<decltype(m_transaction_file)>()) | ||
| ("p,controlpkt", "Control packet binary", cxxopts::value<decltype(m_controlpkt_file)>()) | ||
| ("j,json", "control packet Patching json file", cxxopts::value<decltype(m_external_buffers_file)>()) | ||
| ("l,lib", "linked libs", cxxopts::value<decltype(m_libs)>()) |
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.
missed adding log_level here
| try { | ||
| all_options.add_options() | ||
| ("o,outputelf", "ELF output file name", cxxopts::value<decltype(output_elffile)>()) | ||
| ("j,json", "control packet Patching json file", cxxopts::value<decltype(json_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.
missed adding log_level here
| ("o,outputelf", "ELF output file name", cxxopts::value<decltype(output_elffile)>()) | ||
| ("j,json", "control packet Patching json file", cxxopts::value<decltype(json_file)>()) | ||
| ("f,flag", "flags", cxxopts::value<decltype(flags)>()) | ||
| ("f,flag", "flags (e.g., 'verbose' for detailed output, 'disabledump', 'fulldump')", cxxopts::value<decltype(flags)>()) |
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.
we dont need verbose here, we can merge verbose functionality with fulldump for aie2ps/aie4/aie2ps_config/aie4_config
| debug_flag = asm_dump_flag::disable; | ||
| else if (flag == full_dump_map) | ||
| toutput->set_debug(asm_dump_flag::full); | ||
| debug_flag = asm_dump_flag::full; |
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.
we dont need verbose.
if (flag == disable_dump_map)
debug_flag = asm_dump_flag::disable;
else if (flag == full_dump_map) {
debug_flag = asm_dump_flag::full;
enable_verbose_logging();
}
| { | ||
| if (lib == legacydpuxclbin) | ||
| arg_offset = 1; | ||
| else if (lib == "verbose") |
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.
how you are passing this flag from aiebu-asm, that code is missing
| if (result.count("log-level")) { | ||
| log_level_str = result["log-level"].as<decltype(log_level_str)>(); | ||
| if (log_level_str == "error") | ||
| aiebu::set_log_level(aiebu::log_level::error); |
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.
how you will set log_level when user is calling lib apis?
| if (result.count("log-level")) { | ||
| log_level_str = result["log-level"].as<decltype(log_level_str)>(); | ||
| if (log_level_str == "error") | ||
| aiebu::set_log_level(aiebu::log_level::error); |
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.
ditto
Signed-off-by: Soham Donwalkar <[email protected]>
Problem solved by the commit
aiebu-asm was printing information to console output while assembling. This could reduce aiebu-asm performance.
Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered
N/A
How problem was solved, alternative solutions (if any) and why they were rejected
1> Added a verbose flag to aiebu-asm utility to enable verbose logs on cout.
2> To further enhance logging, added log levels for logging, enabled by aiebu-asm flags. "warn" is the default level.
Added --log-level option to both aie4 and config targets:
("log-level", "log level: error, warn, info, debug (default: warn)"
The flag accepts 4 values:
Below are Log levels and its contents.
Example Usage -
Risks (if any) associated the changes in the commit
N/A
What has been tested and how, request additional testing if necessary
1> Tested on Windows aiebu-asm utility
Documentation impact (if any)
N/A