[Comgr] Create a Global Logger Class for Comgr by theSK2005 · Pull Request #3116 · ROCm/llvm-project

theSK2005 · 2026-06-29T17:49:20Z

Addresses this Jira Issue https://amd-hub.atlassian.net/browse/LCOMPILER-1886

Creates a COMGR::Logger class with a global instance to allow all Comgr APIs to access the same set of logging utilities
Creates separate log streams and APIs and allows logging granularity to be toggleable
Replaces logging in comgr.cpp with new logging scheme
Allows thread-safe access to log streams with mutexes/locks

MixedMatched

Just a few things. Overall great work!

chinmaydd · 2026-06-29T22:42:56Z

Nit: Title should be updated with prefix [Comgr]

chinmaydd · 2026-06-29T22:44:58Z

+  static const char *LogLevel = getenv("AMD_COMGR_LOG_LEVEL");
+  return LogLevel ? StringRef(LogLevel) : StringRef();


We probably want to be case-insensitive here ?

Also, whats the rationale behind using strings over numbers to denote severity ?

I would lean more to using numbers since thats what we currently do in runtime.

Its also easier to think about IMO

Internally Comgr can map an integer to a severity level. But externally I think we should just expose them as numbers

Refer: https://rocm.docs.amd.com/projects/HIP/en/latest/how-to/logging.html

the case insensitivity is handled in comgr-logger.cpp, are you saying to move that the case lower to here?

I suppose the sensitivity discussion is irrelevant if we're exposing numbers instead

Oh sorry, when I posted that comment, I didn't see the more recent ones. Personally, I think that these labels make it more clear for both assigning LogLevels to a particular output and for toggling the threshold for what logs a user would like to see.

I disagree. There's a few reasons:

The precedent has already been set by various other projects in the ROCm / HIP ecosystem. See 1, 2, 3 etc. Since Comgr is a member of the ROCm ecosystem we want to play along.

Its intuitive to think about "big number = more logging". This way you dont have to worry about the exact number assigned to the level of logging. You can just say COMGR_LOG_LEVEL = 100 and the user gets what they want. Very likely, they are not going to be choosing between these levels. They'd want the maximum possible logging when debugging an error. In fact, I have been using AMD_LOG_LEVEL=7 for runtime and I have no clue what the individual numbers map to.

Tangential to point 2, you cant really establish an intuitive relationship between the various levels. How are "info" and "warning" related ? Does "error" give me more or less logging than "debug" ? Its confusing.

That makes sense, thanks for the clarification. I'll do a 0-20 level logging and let the users have autonomy.

We dont want to do 0-100 exactly. We still want to do 0-4 or 0-5 (depending on the available levels).

However, we do want to perform numeric comparison rather than equality when checking against the parsed value.

chinmaydd · 2026-06-29T22:49:27Z

+    return LogLevel::None;
+  if (Requested.equals_insensitive("error"))
+    return LogLevel::Error;
+  if (Requested.equals_insensitive("warning"))
+    return LogLevel::Warning;
+  if (Requested.equals_insensitive("info"))
+    return LogLevel::Info;
+  if (Requested.equals_insensitive("debug"))
+    return LogLevel::Debug;
+


Commented out code should be removed

chinmaydd · 2026-06-30T22:27:37Z

Nice, we are starting to get there!

Whats the reasoning behind the 0-20? We should just advertise the 4 levels (1-4) and leave everything else to numeric comparison. I'm counting the 0 here as equivalent to LOG_LEVEL_NONE. Named enums work great in this case.

We should take inspiration from other projects in the ecosystem that I've linked.

I'm not a fan of the verbose comment strategy that LLMs have. We should keep them to a minimum unless absolutely necessary.

Some other review points (LLM generated, but I think they make sense):

Fix the AMD_COMGR_LOG_LEVEL docs/API mismatch: README.md and amd_comgr.h.in document symbolic values, but parseLogLevel only accepts numeric 0-20 values.
Report redirect-open failures independently of Logger::emit(5, ...), since low or zero log levels can now suppress a diagnostic that used to reach comgr.log.
Add an integration test through amd_comgr_do_action for AMD_COMGR_REDIRECT_LOGS plus AMD_COMGR_LOG_LEVEL; the new tests only cover Logger in isolation.
Add coverage that AMD_COMGR_LOG_LEVEL takes precedence over AMD_COMGR_EMIT_VERBOSE_LOGS, especially for level 0 and invalid values.
Replace raw severity literals like 5 and 20 with named constants so call sites communicate intent.
Use unsigned or uint32_t for LogLevel instead of uint8_t to avoid char-like behavior and unnecessary narrowing.
Bind Logger &Log = getLogger(); once in amd_comgr_do_action instead of repeating getLogger() throughout the block.
Make TeeStream take a Logger & explicitly instead of reaching back to the global logger inside write_impl.
Avoid exposing both getSink() and writeToSink() if direct sink writes are unsafe; prefer one higher-level logging/tee API.
Trim the large explanatory comments in comgr-logger.h, comgr.cpp, and test-unit/CMakeLists.txt; several restate the implementation rather than clarifying it.
Keep the public docs and source comments aligned with the final numeric AMD_COMGR_LOG_LEVEL design, not the old symbolic severity names.
Consider using named test constants for common levels and severities in LoggerTest.cpp instead of repeating magic numbers.

lamb-j · 2026-07-01T18:23:23Z

+/// Severity of a log message, and the logger's configured threshold, expressed
+/// on a 0-to-4 scale where 0 silences logging and higher values are more
+/// verbose. A message is emitted only when its severity is non-zero and does
+/// not exceed the configured level (see Logger::isEnabled). Callers choose the
+/// numeric severity passed to Logger::emit; larger numbers are reserved for
+/// more detailed diagnostics.


Same for the other function definitions in this file.

+1 need to shorten more. I think this is mostly okay, but could use some concise language

lamb-j · 2026-07-01T18:31:48Z

Can we use a named enum (None=0, Error=1, Warning=2, Info=3, Debug=4) with the Severity <= Level checks?

lamb-j · 2026-07-01T18:39:59Z

Some LLM comments:

Test coverage gaps:
- Precedence: no test that an explicit AMD_COMGR_LOG_LEVEL beats verbose — e.g. parseLogLevel("0", true) == 0, parseLogLevel("2", true) == 2.
- No integration test driving AMD_COMGR_REDIRECT_LOGS + AMD_COMGR_LOG_LEVEL through amd_comgr_do_action; current tests exercise Logger in
isolation only.
TeeStream should take a Logger & instead of reaching back to getLogger() inside write_impl.
Reconsider exposing both getSink() and writeToSink(). Raw sink access is what enabled the unsynchronized flush at :1445; a single tee/flush
API that owns the lock removes that footgun.
Bind Logger &Log = getLogger() once in amd_comgr_do_action rather than ~6 repeat calls.

theSK2005 · 2026-07-01T20:23:22Z

@lamb-j @chinmaydd my last commits just addressed all of your recent comments, please check when you're able to

chinmaydd · 2026-07-08T18:49:17Z

+}
+
+LogLevel resolveLevel() {
+  static const char *LogLevel = getenv("AMD_COMGR_LOG_LEVEL");


Can we rename this to something other than LogLevel ?

…o address

…opped gating redirect failures on LogLevel not being none

chinmaydd · 2026-07-09T21:04:25Z

+  return static_cast<LogLevel>(std::min(Numeric, Max));
+}
+
+LogLevel resolveLevel() {


Maybe resolveLogLevel or getLogLevel (since that is the convention for other functions here) ? But I wont fight you on it.

chinmaydd · 2026-07-09T21:05:39Z

+    // Capture emit() output from this action into LogS (the buffer backing the
+    // AMD_COMGR_DATA_KIND_LOG object) so it reaches the caller.


Probably not needed

chinmaydd · 2026-07-09T21:10:08Z

+// RUN:   logger-redirect %s 1.2 %t.debug.returned.log
+// RUN: FileCheck %s < %t.debug.log
+// RUN: FileCheck %s < %t.debug.returned.log
+// CHECK: amd_comgr_do_action:


Maybe this test could be stronger -- with the different log levels ? I'd defer to your judgement

chinmaydd

LGTM with nits! Looks great.

@MixedMatched, @lamb-j can you take a look as well ?

lamb-j requested a review from MixedMatched June 29, 2026 18:22

MixedMatched requested changes Jun 29, 2026

View reviewed changes

theSK2005 marked this pull request as ready for review June 29, 2026 22:39

theSK2005 requested review from chinmaydd and lamb-j as code owners June 29, 2026 22:39

theSK2005 changed the title ~~Create a Global Logger Class for Comgr~~ [Comgr] Create a Global Logger Class for Comgr Jun 29, 2026

chinmaydd reviewed Jun 29, 2026

View reviewed changes

Comment thread amd/comgr/src/comgr-logger.cpp

chinmaydd reviewed Jun 29, 2026

View reviewed changes

chinmaydd added the comgr Related to Code Object Manager label Jun 29, 2026

chinmaydd requested changes Jun 30, 2026

View reviewed changes

theSK2005 requested review from MixedMatched and chinmaydd June 30, 2026 16:58