[llvm-branch-commits] [mlir] [MLIR][Remark] Make remark reporting thread-safe and order final remarks by source position (PR #227360)
Henrich Lauko via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Wed Sep 30 03:09:26 PDT 2026
https://github.com/xlauko updated https://github.com/llvm/llvm-project/pull/227360
>From 403a58797d341cee1658381a71519b8913d25489 Mon Sep 17 00:00:00 2001
From: Henrich Lauko <hlauko at nvidia.com>
Date: Tue, 29 Sep 2026 15:51:08 +0000
Subject: [PATCH] [MLIR][Remark] Make remark reporting thread-safe and order
final remarks by source position
Passes nested under the multithreaded pass manager report into the same
RemarkEngine from worker threads, but nothing in the engine or the
policies was synchronized. RemarkEmittingPolicyFinal inserted into its
map from several threads at once, and under RemarkEmittingPolicyAll the
streamer, including the LLVM remark serializer, was called from several
threads at once. ThreadSanitizer reports both races in the new unit tests.
RemarkEngine now holds a lock around every call into the policy. It is a
recursive llvm::sys::SmartMutex, the same type as the DiagnosticEngine's
lock. report() takes it, and so does a new finalizePolicy(), which the
engine destructor and mlir-opt now use instead of calling finalize() on
the policy directly. For the All policy the streamer and the diagnostic
printer also run under the lock, so custom policies and streamers need no
lock of their own.
With the race fixed, the final policy's creation order is still not
deterministic: which thread reports first depends on scheduling.
finalize() now sorts root remarks by source position. The key is the file
positions nested in the location, in walk order, so the first is the one
the diagnostic printer shows and the rest separate one callee inlined at
different call sites. Remark name, category and kind follow, and the
printed location breaks any remaining tie. Remarks without a file position
come last. Identity and deduplication do not change, and linked remarks
still follow their parent.
remark-final.mlir swaps two CHECK lines: its remarks share a location and
a name, so the category now decides their order. New unit tests cover
source order, independence from report order, a linked remark at an
earlier position, and concurrent reporting under both policies. The new
lit test remark-final-parallel.mlir runs test-remark on four functions in
parallel and checks the same output with and without threading.
Assisted-by: Claude Code (Claude Opus 5.5)
---
mlir/docs/Remarks.md | 34 +++-
mlir/include/mlir/IR/Remarks.h | 39 ++++-
mlir/lib/IR/Remarks.cpp | 94 +++++++++--
mlir/lib/Tools/mlir-opt/MlirOptMain.cpp | 2 +-
mlir/test/Pass/remark-final-parallel.mlir | 51 ++++++
mlir/test/Pass/remark-final.mlir | 17 +-
mlir/unittests/IR/RemarkTest.cpp | 189 +++++++++++++++++++++-
7 files changed, 393 insertions(+), 33 deletions(-)
create mode 100644 mlir/test/Pass/remark-final-parallel.mlir
diff --git a/mlir/docs/Remarks.md b/mlir/docs/Remarks.md
index e747f7bc8f595..de967a07c54d0 100644
--- a/mlir/docs/Remarks.md
+++ b/mlir/docs/Remarks.md
@@ -205,8 +205,14 @@ Emits **all** remarks unconditionally.
Stores remarks until `finalize()` is called and emits only the **final** remark
for each location. This is useful in multi-pass compilers where an early pass
may report a failure, but a later pass succeeds. `finalize()` drains the stored
-remarks and emits them in the order in which they were created. Calling it
-again emits only remarks reported since.
+remarks. Calling it again emits only remarks reported since.
+
+Root remarks are emitted sorted by source position: by the file positions
+nested in their location, the first of which is the one the diagnostic printer
+shows, then by remark name, category and kind. Remarks whose location holds no
+file position come last. Linked remarks follow right after the remark that
+references them. The order does not depend on the order in which remarks were
+reported, so it is the same whether or not passes run in parallel.
**Example:** Only the successful remark is emitted:
@@ -222,6 +228,26 @@ remark::passed(loc, opts) << "Loop unrolled successfully";
You can also implement custom policies by inheriting from the policy interface.
+### Thread safety
+
+Passes that run in parallel report into the same `RemarkEngine`. The engine
+takes a lock around each call into the policy, so `reportRemark` and
+`finalize`, and the streamer calls a policy makes from them, never run
+concurrently. Custom policies and streamers therefore need no lock of their
+own. They must not report remarks or wait for threads that report remarks,
+and diagnostic handlers must not report remarks either.
+
+To emit the remarks a final policy holds while other threads may still report,
+call `RemarkEngine::finalizePolicy()` rather than calling `finalize()` on
+`getRemarkEmittingPolicy()` directly. The engine destructor finalizes the
+policy as well.
+
+With several threads, some output still depends on scheduling. Under
+`RemarkEmittingPolicyAll`, the streamer receives remarks in the order in which
+threads reach the lock. `RemarkId` and `RelatedTo` values come from a shared
+counter. If two threads report the same identity, whichever reports last
+decides the content the final policy keeps.
+
***
## Querying Enabled Remarks
@@ -316,7 +342,9 @@ remark::enableOptimizationRemarks(
### Option 3: Custom Streamer
-Implement your own backend for specialized output formats:
+Implement your own backend for specialized output formats. The engine calls
+`streamOptimizationRemark` under its lock, so it needs no locking of its own:
+
```c++
class MyStreamer : public MLIRRemarkStreamerBase {
diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h
index 0ecc63717b5ae..d90f04c46c000 100644
--- a/mlir/include/mlir/IR/Remarks.h
+++ b/mlir/include/mlir/IR/Remarks.h
@@ -17,6 +17,7 @@
#include "llvm/IR/DiagnosticInfo.h"
#include "llvm/Remarks/Remark.h"
#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/Mutex.h"
#include "llvm/Support/Regex.h"
#include "mlir/IR/Diagnostics.h"
@@ -449,6 +450,10 @@ class InFlightRemark {
/// optimization remarks to the underlying remark streamer. The derived classes
/// should implement the `streamOptimizationRemark` method to provide the
/// actual streaming implementation.
+///
+/// The RemarkEngine calls `streamOptimizationRemark` under its lock, so an
+/// implementation does not need a lock of its own. It must not report remarks
+/// or wait for threads that report remarks.
class MLIRRemarkStreamerBase {
public:
virtual ~MLIRRemarkStreamerBase() = default;
@@ -468,6 +473,10 @@ using ReportFn = llvm::unique_function<void(const Remark &)>;
/// optimization remarks to the underlying remark streamer. The derived classes
/// should implement the `reportRemark` method to provide the actual emitting
/// implementation.
+///
+/// Through the RemarkEngine, `reportRemark` and `finalize` run under the
+/// engine's lock and are never entered concurrently, even when passes report
+/// remarks from several threads.
class RemarkEmittingPolicyBase {
protected:
ReportFn reportImpl;
@@ -514,6 +523,11 @@ class RemarkEngine {
bool printAsEmitRemarks = false;
/// Atomic counter for generating unique remark IDs.
std::atomic<uint64_t> nextRemarkId{1};
+ /// Serializes report() and finalizePolicy(). Passes running in parallel
+ /// report into the same engine, and neither the policies nor the streamers
+ /// are thread-safe. Recursive, like the DiagnosticEngine's lock, so a
+ /// callback that reports on the same thread does not deadlock.
+ llvm::sys::SmartMutex<true> mutex;
/// Emit a remark using the given maker function, which should return
/// a Remark instance. The remark will be emitted using the main
@@ -547,11 +561,17 @@ class RemarkEngine {
std::unique_ptr<RemarkEmittingPolicyBase> remarkEmittingPolicy,
std::string *errMsg);
- /// Get the remark emitting policy.
+ /// Get the remark emitting policy. Calling into the policy directly
+ /// bypasses the engine's lock; use finalizePolicy() to finalize it while
+ /// other threads may still report remarks.
RemarkEmittingPolicyBase *getRemarkEmittingPolicy() const {
return remarkEmittingPolicy.get();
}
+ /// Finalize the emitting policy under the engine's lock, e.g. to emit the
+ /// remarks a RemarkEmittingPolicyFinal holds once a pipeline has finished.
+ void finalizePolicy();
+
/// Generate a unique ID for a new remark.
RemarkId generateRemarkId() {
return RemarkId(nextRemarkId.fetch_add(1, std::memory_order_relaxed));
@@ -607,7 +627,8 @@ class RemarkEngine {
findRemarks(const RemarkOpts &opts,
std::optional<RemarkKind> kind = std::nullopt) const;
- /// Report a remark.
+ /// Report a remark. Thread-safe: reports from several threads are handed to
+ /// the policy one at a time.
void report(const Remark &&remark);
/// Report a successful remark, this will create an InFlightRemark
@@ -666,11 +687,15 @@ class RemarkEmittingPolicyAll : public detail::RemarkEmittingPolicyBase {
};
/// Policy that emits only the last remark reported for each identity, see
-/// DenseMapInfo<Remark>. Remarks are stored until finalize(), which emits them
-/// in creation order, so the output does not depend on hash order.
+/// DenseMapInfo<Remark>. Remarks are stored until finalize(), where a later
+/// remark with the same identity replaces the stored one. finalize() emits root
+/// remarks sorted by source position, so the output does not depend on the
+/// order in which remarks were reported, or on how parallel passes were
+/// scheduled.
class RemarkEmittingPolicyFinal : public detail::RemarkEmittingPolicyBase {
private:
- /// Remarks reported since the last finalize().
+ /// Remarks reported since the last finalize(). Creation order is only a
+ /// tie-break for finalize()'s sort.
llvm::DenseSet<detail::Remark> postponedRemarks;
public:
@@ -681,7 +706,9 @@ class RemarkEmittingPolicyFinal : public detail::RemarkEmittingPolicyBase {
postponedRemarks.insert(remark);
}
- /// Emits and drains all stored remarks. Related remarks are printed right
+ /// Emits and drains all stored remarks. Root remarks come out sorted by the
+ /// file positions nested in their location (remarks with none last), then
+ /// by remark name, category and kind. Related remarks are printed right
/// after the remark that references them; a link only resolves when both
/// remarks are in the same call. A later call emits only remarks reported
/// since this one.
diff --git a/mlir/lib/IR/Remarks.cpp b/mlir/lib/IR/Remarks.cpp
index 2c6c533bb62ae..a3c18267bfaaf 100644
--- a/mlir/lib/IR/Remarks.cpp
+++ b/mlir/lib/IR/Remarks.cpp
@@ -12,6 +12,8 @@
#include "mlir/IR/Diagnostics.h"
#include "mlir/IR/Value.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/Sequence.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
@@ -262,13 +264,21 @@ void RemarkEngine::reportImpl(const Remark &remark) {
}
void RemarkEngine::report(const Remark &&remark) {
- if (remarkEmittingPolicy)
- remarkEmittingPolicy->reportRemark(remark);
+ if (!remarkEmittingPolicy)
+ return;
+ llvm::sys::SmartScopedLock<true> lock(mutex);
+ remarkEmittingPolicy->reportRemark(remark);
+}
+
+void RemarkEngine::finalizePolicy() {
+ if (!remarkEmittingPolicy)
+ return;
+ llvm::sys::SmartScopedLock<true> lock(mutex);
+ remarkEmittingPolicy->finalize();
}
RemarkEngine::~RemarkEngine() {
- if (remarkEmittingPolicy)
- remarkEmittingPolicy->finalize();
+ finalizePolicy();
if (remarkStreamer)
remarkStreamer->finalize();
@@ -365,6 +375,59 @@ namespace mlir::remark {
RemarkEmittingPolicyAll::RemarkEmittingPolicyAll() = default;
RemarkEmittingPolicyFinal::RemarkEmittingPolicyFinal() = default;
+namespace {
+/// Where RemarkEmittingPolicyFinal::finalize() places a root remark. The
+/// fields depend only on the remark's identity, never on its ID or arguments,
+/// so the order does not depend on which report of an identity was kept or on
+/// the order in which threads reported.
+struct FinalOrderKey {
+ /// Every file position nested in the location, in pre-order walk order. The
+ /// first is the one the diagnostic printer shows; the others tell apart,
+ /// for example, one callee inlined at two call sites.
+ SmallVector<std::tuple<StringRef, unsigned, unsigned>, 2> positions;
+ StringRef remarkName;
+ StringRef categoryName;
+ RemarkKind kind;
+ Location loc;
+ /// The printed location, computed only to break a tie between two
+ /// different locations with the same file positions.
+ mutable std::optional<std::string> printedLoc;
+
+ explicit FinalOrderKey(const detail::Remark &remark)
+ : remarkName(remark.getRemarkName()),
+ categoryName(remark.getCombinedCategoryName()),
+ kind(remark.getRemarkKind()), loc(remark.getLocation()) {
+ loc->walk([&](Location nested) {
+ if (auto flc = dyn_cast<FileLineColLoc>(nested))
+ positions.emplace_back(flc.getFilename().getValue(), flc.getLine(),
+ flc.getColumn());
+ return WalkResult::advance();
+ });
+ }
+
+ StringRef getPrintedLoc() const {
+ if (!printedLoc) {
+ printedLoc.emplace();
+ llvm::raw_string_ostream os(*printedLoc);
+ os << loc;
+ }
+ return *printedLoc;
+ }
+
+ bool operator<(const FinalOrderKey &other) const {
+ // Remarks without any file position go last.
+ if (positions.empty() != other.positions.empty())
+ return other.positions.empty();
+ auto fields = std::tie(positions, remarkName, categoryName, kind);
+ auto otherFields = std::tie(other.positions, other.remarkName,
+ other.categoryName, other.kind);
+ if (fields != otherFields)
+ return fields < otherFields;
+ return getPrintedLoc() < other.getPrintedLoc();
+ }
+};
+} // namespace
+
void RemarkEmittingPolicyFinal::finalize() {
assert(reportImpl && "reportImpl is not set");
@@ -383,21 +446,32 @@ void RemarkEmittingPolicyFinal::finalize() {
llvm::DenseMap<uint64_t, const detail::Remark *> idMap;
llvm::DenseSet<uint64_t> childIds; // IDs referenced as children
- for (const auto &remark : remarks) {
+ for (const detail::Remark &remark : remarks) {
if (remark.getId())
idMap[remark.getId().getValue()] = &remark;
for (auto relId : remark.getRelatedRemarkIds())
childIds.insert(relId.getValue());
}
- // Emit remarks with related remarks grouped after their parents.
- // Parent remarks are emitted first, followed by their related (child)
- // remarks. Child-only remarks are skipped at the top level to avoid
- // duplication.
- for (const auto &remark : remarks) {
+ // Sort the root remarks, those not printed under a parent, by source
+ // position. Sort indices rather than `remarks` itself, since idMap points
+ // into it. The stable sort falls back to creation order only for two
+ // different locations that print the same.
+ SmallVector<unsigned> roots;
+ SmallVector<FinalOrderKey> keys;
+ for (auto [index, remark] : llvm::enumerate(remarks)) {
if (remark.getId() && childIds.contains(remark.getId().getValue()))
continue; // will be printed grouped under its parent
+ roots.push_back(index);
+ keys.emplace_back(remark);
+ }
+ auto order = llvm::to_vector(llvm::seq<unsigned>(0, roots.size()));
+ llvm::stable_sort(order,
+ [&](unsigned a, unsigned b) { return keys[a] < keys[b]; });
+ // Emit remarks with related remarks grouped after their parents.
+ for (unsigned rootIndex : order) {
+ const detail::Remark &remark = remarks[roots[rootIndex]];
reportImpl(remark);
// Emit related remarks immediately after the parent.
diff --git a/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp b/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
index 8b49258e135d2..5f0640209a48b 100644
--- a/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
+++ b/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
@@ -633,7 +633,7 @@ performActions(raw_ostream &os,
// This is required if the remark policy is final. Otherwise, the remarks are
// not emitted.
if (remark::detail::RemarkEngine *engine = ctx.getRemarkEngine())
- engine->getRemarkEmittingPolicy()->finalize();
+ engine->finalizePolicy();
return success();
}
diff --git a/mlir/test/Pass/remark-final-parallel.mlir b/mlir/test/Pass/remark-final-parallel.mlir
new file mode 100644
index 0000000000000..2507070ad31bb
--- /dev/null
+++ b/mlir/test/Pass/remark-final-parallel.mlir
@@ -0,0 +1,51 @@
+// RUN: mlir-opt %s --pass-pipeline='builtin.module(func.func(test-remark))' --remarks-filter-passed=category-1-passed --remark-policy=final -o /dev/null 2>&1 | FileCheck %s --implicit-check-not="remark:"
+// RUN: mlir-opt %s --pass-pipeline='builtin.module(func.func(test-remark))' --remarks-filter-passed=category-1-passed --remark-policy=final --mlir-disable-threading -o /dev/null 2>&1 | FileCheck %s --implicit-check-not="remark:"
+// RUN: mlir-opt %s --pass-pipeline='builtin.module(func.func(test-remark))' --remarks-filter-passed=category-1-passed --remark-policy=final --remark-format=yaml --remarks-output-file=%t.yaml -o /dev/null
+// RUN: FileCheck --check-prefix=CHECK-YAML %s --implicit-check-not="--- !" < %t.yaml
+
+// test-remark runs on each function in parallel and reports one passed remark
+// per operation. The final policy prints them sorted by source position, which
+// differs from both the IR order and any order the threads can report in, and
+// is the same with and without threading. Remarks without a file position come
+// last.
+
+func.func @c() {
+ return loc("b.c":1:1)
+} loc("a.c":30:1)
+func.func @a() {
+ return loc("a.c":11:1)
+} loc("a.c":10:1)
+func.func @b() {
+ return loc("n"("a.c":20:1))
+} loc(fused["a.c":10:5, "x"])
+func.func @d() {
+ return loc(unknown)
+} loc("a.c":25:1)
+
+// CHECK: a.c:10:1: remark: [Passed]
+// CHECK-NEXT: a.c:10:5: remark: [Passed]
+// CHECK-NEXT: a.c:11:1: remark: [Passed]
+// CHECK-NEXT: a.c:20:1: remark: [Passed]
+// CHECK-NEXT: a.c:25:1: remark: [Passed]
+// CHECK-NEXT: a.c:30:1: remark: [Passed]
+// CHECK-NEXT: b.c:1:1: remark: [Passed]
+// CHECK-NEXT: <unknown>:0: remark: [Passed]
+
+// The YAML serializer only resolves a plain file location, so the fused and
+// named ones are written as "<unknown file>", but the order is the same.
+// CHECK-YAML: --- !Passed
+// CHECK-YAML: DebugLoc: { File: a.c, Line: 10, Column: 1 }
+// CHECK-YAML: --- !Passed
+// CHECK-YAML: DebugLoc: { File: '<unknown file>', Line: 0, Column: 0 }
+// CHECK-YAML: --- !Passed
+// CHECK-YAML: DebugLoc: { File: a.c, Line: 11, Column: 1 }
+// CHECK-YAML: --- !Passed
+// CHECK-YAML: DebugLoc: { File: '<unknown file>', Line: 0, Column: 0 }
+// CHECK-YAML: --- !Passed
+// CHECK-YAML: DebugLoc: { File: a.c, Line: 25, Column: 1 }
+// CHECK-YAML: --- !Passed
+// CHECK-YAML: DebugLoc: { File: a.c, Line: 30, Column: 1 }
+// CHECK-YAML: --- !Passed
+// CHECK-YAML: DebugLoc: { File: b.c, Line: 1, Column: 1 }
+// CHECK-YAML: --- !Passed
+// CHECK-YAML: DebugLoc: { File: '<unknown file>', Line: 0, Column: 0 }
diff --git a/mlir/test/Pass/remark-final.mlir b/mlir/test/Pass/remark-final.mlir
index d406de295c551..764991a58f95c 100644
--- a/mlir/test/Pass/remark-final.mlir
+++ b/mlir/test/Pass/remark-final.mlir
@@ -5,24 +5,25 @@ module @foo {
"test.op"() : () -> ()
}
-// Remarks come out in creation order. The two passed remarks in
-// "category-1-passed" share an identity, so only the second survives. mlir-opt
-// calls finalize() explicitly and the engine destructor calls it again. The
-// second call must not emit the remarks a second time. --implicit-check-not
-// pins the number of "remark:" lines and of YAML records to five.
+// The two passed remarks in "category-1-passed" share an identity, so only the
+// second survives. All remarks share a location and a name, so the category
+// decides their order. mlir-opt calls finalize()
+// explicitly and the engine destructor calls it again. The second call must not
+// emit the remarks a second time. --implicit-check-not pins the number of
+// "remark:" lines and of YAML records to five.
// CHECK: remark: [Passed] test-remark | Category:category-1-passed |{{.*}}Remark="This is a test passed remark",
-// CHECK: remark: [Failure] test-remark | Category:category-2-failed
// CHECK: remark: [Analysis] test-remark | Category:category-2-analysis
+// CHECK: remark: [Failure] test-remark | Category:category-2-failed
// CHECK: remark: [Passed] test-remark | Category:category-link |{{.*}}RelatedTo=
// CHECK: remark: [Analysis] test-remark | Category:category-link
// CHECK-YAML: --- !Passed
// CHECK-YAML-NEXT: Pass:{{.*}}category-1-passed
-// CHECK-YAML: --- !Failure
-// CHECK-YAML-NEXT: Pass:{{.*}}category-2-failed
// CHECK-YAML: --- !Analysis
// CHECK-YAML-NEXT: Pass:{{.*}}category-2-analysis
+// CHECK-YAML: --- !Failure
+// CHECK-YAML-NEXT: Pass:{{.*}}category-2-failed
// CHECK-YAML: --- !Passed
// CHECK-YAML-NEXT: Pass:{{.*}}category-link
// CHECK-YAML: --- !Analysis
diff --git a/mlir/unittests/IR/RemarkTest.cpp b/mlir/unittests/IR/RemarkTest.cpp
index fba3d96b4389f..b6ad270699cc7 100644
--- a/mlir/unittests/IR/RemarkTest.cpp
+++ b/mlir/unittests/IR/RemarkTest.cpp
@@ -14,6 +14,7 @@
#include "mlir/Remark/RemarkStreamer.h"
#include "mlir/Support/TypeID.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/Config/llvm-config.h"
#include "llvm/IR/LLVMRemarkStreamer.h"
#include "llvm/Remarks/RemarkFormat.h"
#include "llvm/Support/FileSystem.h"
@@ -21,7 +22,9 @@
#include "llvm/Support/YAMLParser.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
+#include <atomic>
#include <optional>
+#include <thread>
#include <vector>
using namespace mlir;
@@ -469,17 +472,17 @@ TEST(Remark, TestRemarkFinalDrains) {
MLIRContext context;
ASSERT_TRUE(succeeded(enableFinalPolicy(context, emitted, "LoopUnroll")));
Location loc = FileLineColLoc::get(&context, "test.cpp", 1, 5);
- auto *policy = context.getRemarkEngine()->getRemarkEmittingPolicy();
+ remark::detail::RemarkEngine *engine = context.getRemarkEngine();
auto first = remark::RemarkOpts::name("First").category("LoopUnroll");
auto second = remark::RemarkOpts::name("Second").category("LoopUnroll");
remark::passed(loc, first) << "first";
remark::passed(loc, second) << "second";
- policy->finalize();
+ engine->finalizePolicy();
EXPECT_THAT(emitted, ElementsAre("First: first", "Second: second"));
// Nothing pending: a repeated call emits nothing.
- policy->finalize();
+ engine->finalizePolicy();
EXPECT_EQ(emitted.size(), 2u);
// A drained identity can be reported again; it waits for the next call,
@@ -500,7 +503,7 @@ TEST(Remark, TestRemarkFinalLinkAcrossDrains) {
MLIRContext context;
ASSERT_TRUE(succeeded(enableFinalPolicy(context, emitted, "LoopUnroll")));
Location loc = FileLineColLoc::get(&context, "test.cpp", 1, 5);
- auto *policy = context.getRemarkEngine()->getRemarkEmittingPolicy();
+ remark::detail::RemarkEngine *engine = context.getRemarkEngine();
remark::RemarkId analysisId;
{
@@ -510,7 +513,7 @@ TEST(Remark, TestRemarkFinalLinkAcrossDrains) {
analysis << "trip count 128";
analysisId = analysis.getId();
}
- policy->finalize();
+ engine->finalizePolicy();
EXPECT_THAT(emitted, ElementsAre("Analysis: trip count 128"));
remark::passed(loc, remark::RemarkOpts::name("Unroller")
@@ -522,6 +525,182 @@ TEST(Remark, TestRemarkFinalLinkAcrossDrains) {
ElementsAre("Analysis: trip count 128", "Unroller: unrolled"));
}
+// Root remarks come out sorted by file, line and column, then by remark name.
+// Remarks whose location holds no file position go last.
+TEST(Remark, TestRemarkFinalSourceOrder) {
+ std::vector<std::string> emitted;
+ {
+ MLIRContext context;
+ ASSERT_TRUE(succeeded(enableFinalPolicy(context, emitted, "LoopUnroll")));
+ auto at = [&](StringRef file, unsigned line, unsigned col) -> Location {
+ return FileLineColLoc::get(&context, file, line, col);
+ };
+ auto named = [](StringRef name) {
+ return remark::RemarkOpts::name(name).category("LoopUnroll");
+ };
+
+ remark::passed(UnknownLoc::get(&context), named("R")) << "unknown";
+ remark::passed(at("b.cpp", 1, 1), named("R")) << "b.cpp:1:1";
+ remark::passed(at("a.cpp", 9, 1), named("R")) << "a.cpp:9:1";
+ remark::passed(
+ NameLoc::get(StringAttr::get(&context, "n"), at("a.cpp", 5, 1)),
+ named("R"))
+ << "a.cpp:5:1 via NameLoc";
+ remark::passed(at("a.cpp", 2, 7), named("R")) << "a.cpp:2:7";
+ remark::passed(at("a.cpp", 2, 3), named("Zeta")) << "a.cpp:2:3";
+ remark::passed(at("a.cpp", 2, 3), named("Alpha")) << "a.cpp:2:3";
+ }
+ EXPECT_THAT(emitted,
+ ElementsAre("Alpha: a.cpp:2:3", "Zeta: a.cpp:2:3", "R: a.cpp:2:7",
+ "R: a.cpp:5:1 via NameLoc", "R: a.cpp:9:1",
+ "R: b.cpp:1:1", "R: unknown"));
+}
+
+// The order depends only on the set of remarks, not on the order in which
+// they were reported. Code inlined from one callee at two call sites shares
+// the callee's position and is ordered by the call site.
+TEST(Remark, TestRemarkFinalOrderIndependentOfReportOrder) {
+ auto run = [](ArrayRef<unsigned> reportOrder) {
+ std::vector<std::string> emitted;
+ {
+ MLIRContext context;
+ EXPECT_TRUE(succeeded(enableFinalPolicy(context, emitted, "LoopUnroll")));
+ Location callee = FileLineColLoc::get(&context, "helper.h", 3, 1);
+ SmallVector<Location> locs = {
+ CallSiteLoc::get(callee,
+ FileLineColLoc::get(&context, "a.cpp", 20, 1)),
+ CallSiteLoc::get(callee,
+ FileLineColLoc::get(&context, "a.cpp", 10, 1)),
+ FileLineColLoc::get(&context, "a.cpp", 4, 2),
+ FileLineColLoc::get(&context, "a.cpp", 4, 1)};
+ auto opts = remark::RemarkOpts::name("R").category("LoopUnroll");
+ for (unsigned index : reportOrder)
+ remark::passed(locs[index], opts)
+ << ("remark " + std::to_string(index));
+ }
+ return emitted;
+ };
+ std::vector<std::string> forward = run({0, 1, 2, 3});
+ EXPECT_THAT(forward, ElementsAre("R: remark 3", "R: remark 2", "R: remark 1",
+ "R: remark 0"));
+ EXPECT_EQ(forward, run({3, 1, 0, 2}));
+}
+
+// A related remark is printed right after the remark that references it, not
+// at its own position.
+TEST(Remark, TestRemarkFinalChildFollowsParent) {
+ std::vector<std::string> emitted;
+ {
+ MLIRContext context;
+ ASSERT_TRUE(succeeded(enableFinalPolicy(context, emitted, "LoopUnroll")));
+ Location early = FileLineColLoc::get(&context, "test.cpp", 1, 1);
+ Location late = FileLineColLoc::get(&context, "test.cpp", 9, 1);
+
+ remark::RemarkId childId;
+ {
+ auto child = remark::analysis(
+ early, remark::RemarkOpts::name("Child").category("LoopUnroll"));
+ child << "at line 1";
+ childId = child.getId();
+ }
+ remark::passed(early,
+ remark::RemarkOpts::name("Other").category("LoopUnroll"))
+ << "at line 1";
+ remark::passed(late, remark::RemarkOpts::name("Parent")
+ .category("LoopUnroll")
+ .relatedTo(childId))
+ << "at line 9";
+ }
+ EXPECT_THAT(emitted, ElementsAre("Other: at line 1", "Parent: at line 9",
+ "Child: at line 1"));
+}
+
+#if LLVM_ENABLE_THREADS
+// Reports from several threads reach the final policy one at a time, and the
+// emitted order does not depend on how the threads were scheduled.
+TEST(Remark, TestRemarkFinalConcurrent) {
+ constexpr unsigned numThreads = 8, perThread = 64;
+ std::vector<std::string> emitted;
+ {
+ MLIRContext context;
+ ASSERT_TRUE(succeeded(enableFinalPolicy(context, emitted, "LoopUnroll")));
+ SmallVector<Location> locs;
+ for (unsigned line = 1; line <= numThreads * perThread; ++line)
+ locs.push_back(FileLineColLoc::get(&context, "test.cpp", line, 1));
+ Location shared = FileLineColLoc::get(&context, "shared.cpp", 1, 1);
+ auto opts = remark::RemarkOpts::name("R").category("LoopUnroll");
+
+ std::atomic<bool> start{false};
+ std::vector<std::thread> threads;
+ for (unsigned t = 0; t < numThreads; ++t) {
+ threads.emplace_back([&, t] {
+ while (!start.load())
+ std::this_thread::yield();
+ for (unsigned i = 0; i < perThread; ++i) {
+ unsigned line = i * numThreads + t + 1;
+ remark::passed(locs[line - 1], opts)
+ << ("line " + std::to_string(line));
+ }
+ // Every thread reports the same identity; one report survives.
+ remark::passed(shared, opts) << "shared";
+ });
+ }
+ start.store(true);
+ for (std::thread &thread : threads)
+ thread.join();
+ }
+ std::vector<std::string> expected = {"R: shared"};
+ for (unsigned line = 1; line <= numThreads * perThread; ++line)
+ expected.push_back("R: line " + std::to_string(line));
+ EXPECT_EQ(emitted, expected);
+}
+
+// Under the All policy the streamer and the diagnostic printer run for every
+// report; the engine serializes them, so a streamer without a lock of its own
+// sees every remark exactly once.
+TEST(Remark, TestRemarkAllConcurrent) {
+ constexpr unsigned numThreads = 8, perThread = 64;
+ std::vector<std::string> emitted;
+ unsigned printed = 0;
+ {
+ MLIRContext context;
+ ScopedDiagnosticHandler handler(&context, [&](Diagnostic &) {
+ ++printed;
+ return success();
+ });
+ mlir::remark::RemarkCategories cats{
+ /*all=*/std::nullopt, /*passed=*/"LoopUnroll", /*missed=*/std::nullopt,
+ /*analysis=*/std::nullopt, /*failed=*/std::nullopt};
+ ASSERT_TRUE(succeeded(remark::enableOptimizationRemarks(
+ context, std::make_unique<RecordingStreamer>(emitted),
+ std::make_unique<remark::RemarkEmittingPolicyAll>(), cats,
+ /*printAsEmitRemarks=*/true)));
+ Location loc = FileLineColLoc::get(&context, "test.cpp", 1, 1);
+ auto opts = remark::RemarkOpts::name("R").category("LoopUnroll");
+
+ std::atomic<bool> start{false};
+ std::vector<std::thread> threads;
+ for (unsigned t = 0; t < numThreads; ++t) {
+ threads.emplace_back([&, t] {
+ while (!start.load())
+ std::this_thread::yield();
+ for (unsigned i = 0; i < perThread; ++i)
+ remark::passed(loc, opts)
+ << ("remark " + std::to_string(t * perThread + i));
+ });
+ }
+ start.store(true);
+ for (std::thread &thread : threads)
+ thread.join();
+ }
+ std::vector<std::string> expected;
+ for (unsigned index = 0; index < numThreads * perThread; ++index)
+ expected.push_back("R: remark " + std::to_string(index));
+ EXPECT_THAT(emitted, ::testing::UnorderedElementsAreArray(expected));
+ EXPECT_EQ(printed, numThreads * perThread);
+}
+#endif // LLVM_ENABLE_THREADS
+
TEST(Remark, TestArgWithAttribute) {
MLIRContext context;
More information about the llvm-branch-commits
mailing list