https://github.com/xlauko updated https://github.com/llvm/llvm-project/pull/224616
>From 8c3cb983ca4ebe7d3393b5c7abebee3df0aa2175 Mon Sep 17 00:00:00 2001 From: Henrich Lauko <[email protected]> Date: Fri, 18 Sep 2026 13:09:19 +0000 Subject: [PATCH] [MLIR][Remark] Emit final-policy remarks in deterministic order RemarkEmittingPolicyFinal stores remarks in a DenseSet keyed on the location pointer, so the output order depends on heap layout and changes between runs. Twenty runs of mlir/test/Pass/remark-final.mlir gave fourteen different orders, which is why the test uses CHECK-DAG. Store remarks in a MapVector keyed by a new RemarkIdentity: location, remark name, combined category name and kind, the same fields the DenseSet compared. A repeated identity overwrites the stored remark in place, so a remark is printed where its identity was first reported with the content it last had. Root remarks come out in first-report order and linked children still follow their parent. DenseMapInfo<Remark> is removed. RemarkIdentity is now the one place that says what the final policy treats as the same remark. Behaviour change: order only. The identity is unchanged. remark-final.mlir switches to ordered CHECK lines. New unit tests cover first-position-last-content replacement, and the identity fields that had no coverage: the unnamed-remark placeholder and the combined category name. Assisted-by: Claude Code (Claude Fable 5.1) --- mlir/docs/Remarks.md | 26 +++++---- mlir/include/mlir/IR/Remarks.h | 94 +++++++++++++++++--------------- mlir/lib/IR/Remarks.cpp | 8 +-- mlir/test/Pass/remark-final.mlir | 34 +++++++----- mlir/unittests/IR/RemarkTest.cpp | 67 ++++++++++++++++++++--- 5 files changed, 151 insertions(+), 78 deletions(-) diff --git a/mlir/docs/Remarks.md b/mlir/docs/Remarks.md index 3f468e730003d5..0b843b46a0509c 100644 --- a/mlir/docs/Remarks.md +++ b/mlir/docs/Remarks.md @@ -202,21 +202,27 @@ Emits **all** remarks unconditionally. ### RemarkEmittingPolicyFinal -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. Calling it again emits only remarks reported since. - -**Example:** Only the successful remark is emitted: +Stores remarks until `finalize()` is called and emits only the **last** remark +reported for each identity. This is useful in multi-pass compilers where several +passes report on the same thing and only the final report should be shown. The +identity is currently the location, remark name, combined category name and +remark kind. Arguments are not part of it. Root remarks are emitted in the order +in which their identity was first reported, with linked remarks right after the +remark that references them, so the output does not depend on hash order. +`finalize()` drains the stored remarks. Calling it again emits only remarks +reported since. + +**Example:** Only the second remark is emitted, because both share an +identity. ```c++ auto opts = remark::RemarkOpts::name("Unroller").category("LoopUnroll"); -// First pass: reports failure -remark::failed(loc, opts) << "Loop could not be unrolled"; +// First attempt. +remark::passed(loc, opts) << "Loop unrolled by 2"; -// Later pass: reports success (this is the one emitted) -remark::passed(loc, opts) << "Loop unrolled successfully"; +// A later pass revisits the same loop. This is the one emitted. +remark::passed(loc, opts) << "Loop unrolled by 4"; ``` You can also implement custom policies by inheriting from the policy interface. diff --git a/mlir/include/mlir/IR/Remarks.h b/mlir/include/mlir/IR/Remarks.h index 61a9f924bca5f9..1a70ca9b1eb19b 100644 --- a/mlir/include/mlir/IR/Remarks.h +++ b/mlir/include/mlir/IR/Remarks.h @@ -13,6 +13,7 @@ #ifndef MLIR_IR_REMARKS_H #define MLIR_IR_REMARKS_H +#include "llvm/ADT/MapVector.h" #include "llvm/ADT/StringExtras.h" #include "llvm/IR/DiagnosticInfo.h" #include "llvm/Remarks/Remark.h" @@ -356,6 +357,48 @@ inline Remark &operator<<(Remark &r, const Remark::Arg &kv) { return r; } +//===----------------------------------------------------------------------===// +// RemarkIdentity +//===----------------------------------------------------------------------===// + +/// The fields RemarkEmittingPolicyFinal compares to decide that two remarks +/// describe the same thing: location, remark name, combined category name and +/// remark kind. Arguments, function name and remark ID are not part of the +/// identity, so a later remark with the same identity replaces an earlier one. +struct RemarkIdentity { + Location loc; + std::string remarkName; + std::string combinedCategoryName; + RemarkKind kind; + + explicit RemarkIdentity(const Remark &remark) + : loc(remark.getLocation()), remarkName(remark.getRemarkName()), + combinedCategoryName(remark.getCombinedCategoryName()), + kind(remark.getRemarkKind()) {} +}; + +} // namespace mlir::remark::detail + +namespace llvm { +template <> +struct DenseMapInfo<mlir::remark::detail::RemarkIdentity> { + using RemarkIdentity = mlir::remark::detail::RemarkIdentity; + + static unsigned getHashValue(const RemarkIdentity &identity) { + return llvm::hash_combine(identity.loc, identity.remarkName, + identity.combinedCategoryName, identity.kind); + } + + static bool isEqual(const RemarkIdentity &lhs, const RemarkIdentity &rhs) { + return lhs.loc == rhs.loc && lhs.kind == rhs.kind && + lhs.remarkName == rhs.remarkName && + lhs.combinedCategoryName == rhs.combinedCategoryName; + } +}; +} // namespace llvm + +namespace mlir::remark::detail { + //===----------------------------------------------------------------------===// // Shorthand aliases for different kinds of remarks. //===----------------------------------------------------------------------===// @@ -665,19 +708,21 @@ class RemarkEmittingPolicyAll : public detail::RemarkEmittingPolicyBase { void finalize() override {} }; -/// Policy that emits only the last remark reported for each identity, see -/// DenseMapInfo<Remark>. Remarks are stored until finalize(). +/// Policy that emits only the last remark reported for each RemarkIdentity. +/// Remarks are stored until finalize(). A later remark with the same identity +/// replaces the stored one in place, so root remarks are emitted in the order +/// in which their identity was first reported. class RemarkEmittingPolicyFinal : public detail::RemarkEmittingPolicyBase { private: - /// Remarks reported since the last finalize(). - llvm::DenseSet<detail::Remark> postponedRemarks; + /// Remarks reported since the last finalize(), keyed by identity and kept + /// in first-report order. + llvm::MapVector<detail::RemarkIdentity, detail::Remark> postponedRemarks; public: RemarkEmittingPolicyFinal(); void reportRemark(const detail::Remark &remark) override { - postponedRemarks.erase(remark); - postponedRemarks.insert(remark); + postponedRemarks.insert_or_assign(detail::RemarkIdentity(remark), remark); } /// Emits and drains all stored remarks. Related remarks are printed right @@ -761,41 +806,4 @@ LogicalResult enableOptimizationRemarks( } // namespace mlir::remark -// DenseMapInfo specialization for Remark -namespace llvm { -template <> -struct DenseMapInfo<mlir::remark::detail::Remark> { - static constexpr StringRef kEmptyKey = "<EMPTY_KEY>"; - - /// Helper to provide a static dummy context for sentinel keys. - static mlir::MLIRContext *getStaticDummyContext() { - static mlir::MLIRContext dummyContext; - return &dummyContext; - } - - /// Create an empty remark - /// Compute the hash value of the remark - static unsigned getHashValue(const mlir::remark::detail::Remark &remark) { - return llvm::hash_combine( - remark.getLocation().getAsOpaquePointer(), - llvm::hash_value(remark.getRemarkName()), - llvm::hash_value(remark.getCombinedCategoryName()), - static_cast<unsigned>(remark.getRemarkKind())); - } - - static bool isEqual(const mlir::remark::detail::Remark &lhs, - const mlir::remark::detail::Remark &rhs) { - // Check for empty keys first. - if (lhs.getRemarkName() == kEmptyKey || rhs.getRemarkName() == kEmptyKey) { - return lhs.getRemarkName() == rhs.getRemarkName(); - } - - // For regular remarks, compare key identifying fields - return lhs.getLocation() == rhs.getLocation() && - lhs.getRemarkName() == rhs.getRemarkName() && - lhs.getCombinedCategoryName() == rhs.getCombinedCategoryName() && - lhs.getRemarkKind() == rhs.getRemarkKind(); - } -}; -} // namespace llvm #endif // MLIR_IR_REMARKS_H diff --git a/mlir/lib/IR/Remarks.cpp b/mlir/lib/IR/Remarks.cpp index 57e43b8a0d090c..4b295827f11e71 100644 --- a/mlir/lib/IR/Remarks.cpp +++ b/mlir/lib/IR/Remarks.cpp @@ -12,6 +12,7 @@ #include "mlir/IR/Diagnostics.h" #include "mlir/IR/Value.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/StringExtras.h" #include "llvm/ADT/StringRef.h" @@ -370,14 +371,13 @@ void RemarkEmittingPolicyFinal::finalize() { // Take the pending remarks so that a second finalize(), e.g. from the engine // destructor after an explicit call, does not emit them again. - llvm::DenseSet<detail::Remark> remarks; - remarks.swap(postponedRemarks); + auto remarks = postponedRemarks.takeVector(); // Build ID -> Remark* lookup for resolving related remark references. 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 : llvm::make_second_range(remarks)) { if (remark.getId()) idMap[remark.getId().getValue()] = &remark; for (auto relId : remark.getRelatedRemarkIds()) @@ -388,7 +388,7 @@ void RemarkEmittingPolicyFinal::finalize() { // 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) { + for (const detail::Remark &remark : llvm::make_second_range(remarks)) { if (remark.getId() && childIds.count(remark.getId().getValue())) continue; // will be printed grouped under its parent diff --git a/mlir/test/Pass/remark-final.mlir b/mlir/test/Pass/remark-final.mlir index e672006a075154..47c132a682aa24 100644 --- a/mlir/test/Pass/remark-final.mlir +++ b/mlir/test/Pass/remark-final.mlir @@ -5,19 +5,25 @@ module @foo { "test.op"() : () -> () } -// 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, in the first one's position. 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-DAG: remark: [Passed] test-remark | Category:category-1-passed |{{.*}}Remark="This is a test passed remark", -// CHECK-DAG: remark: [Failure] test-remark | Category:category-2-failed -// CHECK-DAG: remark: [Analysis] test-remark | Category:category-2-analysis -// CHECK-DAG: remark: [Passed] test-remark | Category:category-link |{{.*}}RelatedTo= -// CHECK-DAG: remark: [Analysis] test-remark | Category:category-link +// 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: [Passed] test-remark | Category:category-link |{{.*}}RelatedTo= +// CHECK: remark: [Analysis] test-remark | Category:category-link -// CHECK-YAML-DAG: --- !Passed -// CHECK-YAML-DAG: --- !Failure -// CHECK-YAML-DAG: --- !Analysis -// CHECK-YAML-DAG: --- !Passed -// CHECK-YAML-DAG: --- !Analysis +// 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: --- !Passed +// CHECK-YAML-NEXT: Pass:{{.*}}category-link +// CHECK-YAML: --- !Analysis +// CHECK-YAML-NEXT: Pass:{{.*}}category-link diff --git a/mlir/unittests/IR/RemarkTest.cpp b/mlir/unittests/IR/RemarkTest.cpp index b578e8c725aa64..df57a45acac995 100644 --- a/mlir/unittests/IR/RemarkTest.cpp +++ b/mlir/unittests/IR/RemarkTest.cpp @@ -428,11 +428,11 @@ class RecordingStreamer : public remark::detail::MLIRRemarkStreamerBase { static LogicalResult enableFinalPolicy(MLIRContext &context, std::vector<std::string> &emitted, - StringRef category) { + StringRef passedCategory) { mlir::remark::RemarkCategories cats{/*all=*/std::nullopt, - /*passed=*/category.str(), + /*passed=*/passedCategory.str(), /*missed=*/std::nullopt, - /*analysis=*/category.str(), + /*analysis=*/passedCategory.str(), /*failed=*/std::nullopt}; return remark::enableOptimizationRemarks( context, std::make_unique<RecordingStreamer>(emitted), @@ -440,6 +440,25 @@ static LogicalResult enableFinalPolicy(MLIRContext &context, /*printAsEmitRemarks=*/false); } +// The final policy emits remarks in the order their identity was first +// reported, with the content of the last report for that identity. +TEST(Remark, TestRemarkFinalOrder) { + std::vector<std::string> emitted; + { + MLIRContext context; + ASSERT_TRUE(succeeded(enableFinalPolicy(context, emitted, "LoopUnroll"))); + Location locA = FileLineColLoc::get(&context, "test.cpp", 1, 5); + Location locB = FileLineColLoc::get(&context, "test.cpp", 2, 5); + auto opts = remark::RemarkOpts::name("Unroller").category("LoopUnroll"); + + remark::passed(locA, opts) << "A first"; + remark::passed(locB, opts) << "B"; + // The function name is not part of the identity. + remark::passed(locA, opts.function("other")) << "A last"; + } + EXPECT_THAT(emitted, ElementsAre("Unroller: A last", "Unroller: B")); +} + // finalize() drains the stored remarks. mlir-opt calls it explicitly and the // engine destructor calls it again. Each call emits only the remarks reported // since the previous one, and an identity drained by one call can be reported @@ -457,8 +476,7 @@ TEST(Remark, TestRemarkFinalDrains) { remark::passed(loc, first) << "first"; remark::passed(loc, second) << "second"; policy->finalize(); - EXPECT_THAT(emitted, - UnorderedElementsAre("First: first", "Second: second")); + EXPECT_THAT(emitted, ElementsAre("First: first", "Second: second")); // Nothing is pending, so a repeated call emits nothing. policy->finalize(); @@ -469,8 +487,43 @@ TEST(Remark, TestRemarkFinalDrains) { remark::passed(loc, first) << "first again"; EXPECT_EQ(emitted.size(), 2u); } - ASSERT_EQ(emitted.size(), 3u); - EXPECT_EQ(emitted[2], "First: first again"); + EXPECT_THAT(emitted, ElementsAre("First: first", "Second: second", + "First: first again")); +} + +// Identity uses the same accessors as the printed remark. An empty name is the +// "<unknown remark name>" placeholder, and category plus sub-category compare +// as their combined "category:sub" form. +TEST(Remark, TestRemarkFinalIdentityFields) { + std::vector<std::string> emitted; + { + MLIRContext context; + ASSERT_TRUE(succeeded(enableFinalPolicy(context, emitted, "Loop.*"))); + Location loc = FileLineColLoc::get(&context, "test.cpp", 1, 5); + + // Two unnamed remarks share an identity. + remark::passed(loc, remark::RemarkOpts::name("").category("LoopUnroll")) + << "unnamed 1"; + remark::passed(loc, remark::RemarkOpts::name("").category("LoopUnroll")) + << "unnamed 2"; + + // Category and sub-category form one combined name. + remark::passed(loc, remark::RemarkOpts::name("Vec") + .category("LoopVectorize") + .subCategory("inner")) + << "combined 1"; + remark::passed(loc, remark::RemarkOpts::name("Vec") + .category("LoopVectorize") + .subCategory("inner")) + << "combined 2"; + // A different sub-category is a different identity. + remark::passed(loc, remark::RemarkOpts::name("Vec") + .category("LoopVectorize") + .subCategory("outer")) + << "outer"; + } + EXPECT_THAT(emitted, ElementsAre("<unknown remark name>: unnamed 2", + "Vec: combined 2", "Vec: outer")); } // A RelatedTo link only resolves between remarks drained by the same _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
