Skip to content
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
1 change: 1 addition & 0 deletions amd/comgr/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ set(SOURCES
src/comgr-hotswap-patch-wmma-scale16.cpp
src/comgr-hotswap-patch-wmma-split.cpp
src/comgr-libcxx-headers.cpp
src/comgr-logger.cpp
src/comgr-metadata.cpp
src/comgr-signal.cpp
src/comgr-spirv-command.cpp
Expand Down
16 changes: 13 additions & 3 deletions amd/comgr/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -213,11 +213,21 @@ include:
to Clang Driver invocations.
* `AMD_COMGR_REDIRECT_LOGS`: If this is not set, or is set to "0", logs are
returned to the caller as normal. If this is set to "stdout"/"-" or "stderr",
logs are instead redirected to the standard output or error stream,
logs are additionally copied to the standard output or error stream,
respectively. If this is set to any other value, it is interpreted as a

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should explain what the log levels denote

filename which logs should be appended to.
filename which logs are appended to. In all cases logs are still returned to
the caller; this variable copies them, it does not move them.
* `AMD_COMGR_EMIT_VERBOSE_LOGS`: If this is set, and is not "0", logs will
include additional Comgr-specific informational messages.
include additional Comgr-specific informational messages. Equivalent to
`AMD_COMGR_LOG_LEVEL=4`, unless `AMD_COMGR_LOG_LEVEL` is set, which takes
precedence.
* `AMD_COMGR_LOG_LEVEL`: Sets the severity threshold of the Comgr logger as an
integer in the range [0, 4], where 0 disables logging and higher values are
more verbose. A message is emitted only when its severity does not exceed this
threshold. Values outside the range are clamped to it; non-integer values are
ignored. Takes precedence over `AMD_COMGR_EMIT_VERBOSE_LOGS`; if unset (or not
an integer), defaults to 4 when `AMD_COMGR_EMIT_VERBOSE_LOGS` is enabled, else
1.
* `AMD_COMGR_TIME_STATISTICS`: If this is set, and is not "0", logs will
include additional Comgr-specific timing information for compilation actions.
* `AMD_COMGR_TIME_STATISTICS_GRANULARITY`: If this is set to "us" or "ns",
Expand Down
18 changes: 14 additions & 4 deletions amd/comgr/include/amd_comgr.h.in
Original file line number Diff line number Diff line change
Expand Up @@ -142,12 +142,22 @@ extern "C" {
* forwards "--save-temps=obj" to Clang Driver invocations
* - @p AMD_COMGR_REDIRECT_LOGS: If this is not set, or is set to "0", logs are
* returned to the caller as normal. If this is set to "stdout"/"-" or
* "stderr", logs are instead redirected to the standard output or error
* "stderr", logs are additionally copied to the standard output or error
* stream, respectively. If this is set to any other value, it is interpreted
* as a filename which logs should be appended to. Logs may be redirected
* irrespective of whether logging is enabled.
* as a filename which logs are appended to. In all cases logs are still
* returned to the caller; this variable copies them, it does not move them.
* Logs may be redirected irrespective of whether logging is enabled.
* - @p AMD_COMGR_EMIT_VERBOSE_LOGS: If this is set, and is not "0", logs will
* include additional Comgr-specific informational messages.
* include additional Comgr-specific informational messages. Equivalent to
* AMD_COMGR_LOG_LEVEL=4, unless AMD_COMGR_LOG_LEVEL is set, which takes
* precedence.
* - @p AMD_COMGR_LOG_LEVEL: Sets the severity threshold of the Comgr logger as
* an integer in the range [0, 4], where 0 disables logging and higher values
* are more verbose. A message is emitted only when its severity does not
* exceed this threshold. Values outside the range are clamped to it;
* non-integer values are ignored. Takes precedence over
* AMD_COMGR_EMIT_VERBOSE_LOGS; if unset (or not an integer), defaults to 4
* when AMD_COMGR_EMIT_VERBOSE_LOGS is enabled, else 1.
*/

/** \defgroup symbol_versions_group Symbol Versions
Expand Down
5 changes: 5 additions & 0 deletions amd/comgr/src/comgr-env.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ bool shouldEmitVerboseLogs() {
return VerboseLogs && StringRef(VerboseLogs) != "0";
}

StringRef getLogLevel() {
static const char *LogLevel = getenv("AMD_COMGR_LOG_LEVEL");
return LogLevel ? StringRef(LogLevel) : StringRef();
Comment on lines +88 to +89

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We probably want to be case-insensitive here ?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@chinmaydd chinmaydd Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@theSK2005 theSK2005 Jun 30, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LogLevel should be an enum as @lamb-j mentioned. This function should return that enum

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Check how Sam is trying to do something similar at #2478

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

just to confirm, this means to revert LogLevel away from using numbers (suggested here #3116 (comment)) and back to using the original None, Error, Warning, Debug, Info enum, correct?

}

llvm::StringRef getLLVMPath() {
static const char *EnvLLVMPath = std::getenv("LLVM_PATH");
return EnvLLVMPath;
Expand Down
5 changes: 5 additions & 0 deletions amd/comgr/src/comgr-env.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ std::optional<llvm::StringRef> getRedirectLogs();
/// Return whether the environment requests verbose logging.
bool shouldEmitVerboseLogs();

/// Return the raw value of AMD_COMGR_LOG_LEVEL (an integer in [0, 4]), or an
/// empty string if unset. The Logger parses this into a numeric severity
/// threshold; see COMGR::parseLogLevel.
llvm::StringRef getLogLevel();

/// Return whether the environment requests time statistics collection.
bool needTimeStatistics();

Expand Down
134 changes: 134 additions & 0 deletions amd/comgr/src/comgr-logger.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
//===- comgr-logger.cpp - Global Comgr logging facility -------------------===//
//
// Part of Comgr, under the Apache License v2.0 with LLVM Exceptions. See
// amd/comgr/LICENSE.TXT in this repository for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
///
/// \file
/// This file implements COMGR::Logger. See comgr-logger.h for the design.
///
//===----------------------------------------------------------------------===//

#include "comgr-logger.h"
#include "comgr-env.h"

#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/FileSystem.h"

using namespace llvm;

namespace COMGR {

namespace {

// Maximum value on the severity/level scale; values above this are clamped.
constexpr LogLevel MaxLogLevel = 4;

// The capture stream is per-thread so that a captured Action on one thread does
// not collect log output emitted by an unrelated API on another thread.
thread_local raw_ostream *ThreadCaptureStream = nullptr;

@chinmaydd chinmaydd Jun 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Comgr doesnt support multithreaded execution yet. Do we need thread_local ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Comgr can be run in multiple threads and is thread safe, via a mutex around LLVM calls that cause issues


// Resolve the configured level from the environment. Delegates the mapping to
// the testable parseLogLevel(); kept separate so the Logger constructor reads
// the (cached) environment exactly once.
LogLevel resolveLevel() {
return parseLogLevel(env::getLogLevel(), env::shouldEmitVerboseLogs());
}

} // namespace

LogLevel parseLogLevel(StringRef Requested, bool VerboseFallback) {
// When the variable is unset or not a valid integer, default to the most
// verbose level if verbose logs are requested for back-compat with
// AMD_COMGR_EMIT_VERBOSE_LOGS, otherwise to a low level that still shows
// errors.
Comment thread
theSK2005 marked this conversation as resolved.
unsigned Numeric;
if (Requested.getAsInteger(10, Numeric))
return VerboseFallback ? MaxLogLevel : 1;

return Numeric > MaxLogLevel ? MaxLogLevel : static_cast<LogLevel>(Numeric);
}

Logger::Logger() : Level(resolveLevel()), Sink(nullptr) {
std::optional<StringRef> RedirectLogs = env::getRedirectLogs();
if (!RedirectLogs)
return;

StringRef RedirectLog = *RedirectLogs;
if (RedirectLog == "stdout" || RedirectLog == "-") {
Sink = &outs();
} else if (RedirectLog == "stderr") {
Sink = &errs();
} else {
std::error_code EC;
SinkFile = std::make_unique<raw_fd_ostream>(
RedirectLog, EC, sys::fs::OF_Text | sys::fs::OF_Append);
if (EC) {
SinkFile.reset();
// Record the failure rather than writing it to stderr here. The Logger is
// constructed before any action's log buffer exists; the action layer
// surfaces this message into the returned comgr.log via getSinkError(),
// restoring the pre-Logger behavior of reporting it to the caller.
Comment on lines +69 to +72

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

shorten?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

+1 need to shorten more

SinkError = (Twine("unable to redirect log to file '") + RedirectLog +
"': " + EC.message())
.str();
} else {
Sink = SinkFile.get();
}
}
}

Logger::Logger(LogLevel Level, raw_ostream *Sink) : Level(Level), Sink(Sink) {}

void Logger::writeToSink(StringRef Data) {
if (!Sink)
return;

// Share the same mutex as emit() so teed output and Logger messages never
// interleave mid-write on the shared sink. The sink is intentionally left
// unflushed here; the caller flushes once when the action completes.
std::scoped_lock<std::mutex> Lock(Mutex);
Comment thread
theSK2005 marked this conversation as resolved.
*Sink << Data;
}

void Logger::emit(LogLevel Severity, const Twine &Message) {
if (!isEnabled(Severity))
return;

SmallString<256> Buffer;
StringRef Text = Message.toStringRef(Buffer);
StringRef Prefix = "comgr: ";

std::scoped_lock<std::mutex> Lock(Mutex);

raw_ostream *Capture = ThreadCaptureStream;
if (Sink) {
*Sink << Prefix << Text << '\n';
Sink->flush();
}
// Guard against double-emission if a capture stream happens to alias the
// sink.
if (Capture && Capture != Sink) {
*Capture << Prefix << Text << '\n';
Capture->flush();
}
}

Logger &getLogger() {
static Logger TheLogger;
return TheLogger;
}

raw_ostream *getThreadCaptureStream() { return ThreadCaptureStream; }

LogCaptureScope::LogCaptureScope(raw_ostream &OS)
: Previous(ThreadCaptureStream) {
ThreadCaptureStream = &OS;
}

LogCaptureScope::~LogCaptureScope() { ThreadCaptureStream = Previous; }

} // namespace COMGR
153 changes: 153 additions & 0 deletions amd/comgr/src/comgr-logger.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
//===- comgr-logger.h - Global Comgr logging facility ---------------------===//
//
// Part of Comgr, under the Apache License v2.0 with LLVM Exceptions. See
// amd/comgr/LICENSE.TXT in this repository for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
///
/// \file
/// This file declares COMGR::Logger, a process-global, thread-safe logging
/// facility shared by every Comgr API. Any API can emit diagnostics at a
/// configurable severity through Logger::emit, passing a numeric severity on a
/// 0-to-4 scale.
///
/// Output goes to two independent destinations:
/// - The global "sink": resolved once from AMD_COMGR_REDIRECT_LOGS (stdout,
/// stderr, or an appended file).
/// - An optional per-thread "capture" stream: installed via LogCaptureScope
/// so emitted messages are also collected into the AMD_COMGR_DATA_KIND_LOG
/// ("comgr.log") data object returned to the caller.
///
/// All writes are guarded by a mutex, so concurrent callers share the sink
/// safely. The severity threshold (a value in [0, 4]) is configured via
/// AMD_COMGR_LOG_LEVEL; see COMGR::env::getLogLevel().
///
//===----------------------------------------------------------------------===//

#ifndef COMGR_LOGGER_H
#define COMGR_LOGGER_H

#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/Twine.h"
#include "llvm/Support/raw_ostream.h"

#include <cstdint>
#include <memory>
#include <mutex>
#include <string>

namespace COMGR {

/// 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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

shorten?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same for the other function definitions in this file.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

using LogLevel = uint8_t;

/// Parse @p Requested (the value of AMD_COMGR_LOG_LEVEL, which may be empty) into
/// a threshold on the 0-to-4 scale. The value must be a bare integer; it is
/// clamped to [0, 4]. When @p Requested is empty or is not a valid integer,
/// returns 4 if @p VerboseFallback is set (back-compat with
/// AMD_COMGR_EMIT_VERBOSE_LOGS), otherwise 1. Exposed for testing.
LogLevel parseLogLevel(llvm::StringRef Requested, bool VerboseFallback);

/// Process-global, thread-safe logging facility. Obtain the shared instance
/// through getLogger(); do not construct directly except for tests.
class Logger {
public:
/// Construct a Logger configured from the environment (AMD_COMGR_LOG_LEVEL
/// and AMD_COMGR_REDIRECT_LOGS). Used for the process-global instance.
Logger();

/// Construct a Logger with an explicit level and non-owning sink (which may
/// be null). Intended for tests.
Logger(LogLevel Level, llvm::raw_ostream *Sink);

Logger(const Logger &) = delete;
Logger &operator=(const Logger &) = delete;

/// Return whether a message of the given @p Severity would be emitted under
/// the current level. A severity of 0 is never emitted, and emission is
/// disabled entirely when the level is 0. Callers that build expensive
/// messages can guard their formatting with this.
bool isEnabled(LogLevel Severity) const {
return Severity != 0 && Level != 0 && Severity <= Level;
}

/// The currently configured maximum severity that will be emitted.
LogLevel getLevel() const { return Level; }

/// Return the global sink stream, resolved once from AMD_COMGR_REDIRECT_LOGS
/// (stdout, stderr, or an appended file), or null when logs are not
/// redirected. Callers that maintain their own per-action log stream can
/// reuse this to avoid opening the redirect destination a second time.
/// Direct writes to the returned stream are NOT serialized against emit();
/// callers that tee output into the sink should go through writeToSink() so
/// they share the logger's mutex.
llvm::raw_ostream *getSink() const { return Sink; }

/// Return a diagnostic describing why the redirect sink could not be opened,
/// or an empty string when redirection was not requested or succeeded. The
/// Logger is constructed before any per-action log buffer exists, so the
/// action layer surfaces this into the returned comgr.log when getSink() is
/// null despite AMD_COMGR_REDIRECT_LOGS being set.
llvm::StringRef getSinkError() const { return SinkError; }

/// Write @p Data verbatim to the global sink while holding the logger's mutex,
/// so callers that tee their own output into the sink (see TeeStream in
/// comgr.cpp) serialize with emit() instead of racing on the shared stream.
/// No prefix or newline is added and the sink is not flushed (the caller is
/// expected to flush once it is done). A no-op when there is no sink.
void writeToSink(llvm::StringRef Data);

/// Emit @p Message at @p Severity, prefixed and newline-terminated. Writes to
/// the global sink and, when one is installed on the calling thread, the
/// capture stream. Thread-safe.
void emit(LogLevel Severity, const llvm::Twine &Message);

private:
LogLevel Level;

// The global sink, resolved once at construction. Null when logs are not
// redirected (AMD_COMGR_REDIRECT_LOGS unset). When pointing at a file, the
// stream is owned by SinkFile.
llvm::raw_ostream *Sink;
std::unique_ptr<llvm::raw_fd_ostream> SinkFile;

// Diagnostic recorded when AMD_COMGR_REDIRECT_LOGS named a file that could not
// be opened. Empty otherwise. Surfaced to the caller via getSinkError().
std::string SinkError;

// Guards all writes to Sink and to the active capture stream.
std::mutex Mutex;
};

/// Return the process-global Logger instance.
Logger &getLogger();

/// Install a capture stream for the current thread for the duration of this
/// scope. While active, every Logger::emit on this thread also writes into
/// @p OS, in addition to the global sink. Nesting is supported: the previous
/// capture stream (if any) is restored on destruction.
class LogCaptureScope {
public:
explicit LogCaptureScope(llvm::raw_ostream &OS);
~LogCaptureScope();

LogCaptureScope(const LogCaptureScope &) = delete;
LogCaptureScope &operator=(const LogCaptureScope &) = delete;

private:
llvm::raw_ostream *Previous;
};

/// Return the capture stream installed on the calling thread, or null. Exposed
/// for Logger::emit; callers should use LogCaptureScope to manage it.
llvm::raw_ostream *getThreadCaptureStream();

} // namespace COMGR

#endif // COMGR_LOGGER_H
Loading
Loading