Reapply "Use global TimerGroups for both new pass manager and old pass manager timers" (#131173) #131217
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…s manager timers" (llvm#131173) This reverts commit 31ebe66. The reason for the test failure is likely due to `Name2PairMap::getTimerGroup(...)` not holding a lock.
llvmbot
added
clang
alanzhao1
changed the title
|
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-clang Author: Alan Zhao (alanzhao1) ChangesThis reverts commit 31ebe66. The reason for the test failure is likely due to Full diff: https://github.com/llvm/llvm-project/pull/131217.diff 6 Files Affected:
diff --git a/clang/test/Misc/time-passes.c b/clang/test/Misc/time-passes.c
index 395da216aad42..c1669826b2268 100644
--- a/clang/test/Misc/time-passes.c
+++ b/clang/test/Misc/time-passes.c
@@ -19,6 +19,5 @@
// NPM: InstCombinePass{{$}}
// NPM-NOT: InstCombinePass #
// TIME: Total{{$}}
-// NPM: Pass execution timing report
int foo(int x, int y) { return x + y; }
diff --git a/llvm/include/llvm/IR/PassTimingInfo.h b/llvm/include/llvm/IR/PassTimingInfo.h
index 1148399943186..b47ba7f16ef37 100644
--- a/llvm/include/llvm/IR/PassTimingInfo.h
+++ b/llvm/include/llvm/IR/PassTimingInfo.h
@@ -39,8 +39,7 @@ Timer *getPassTimer(Pass *);
/// This class implements -time-passes functionality for new pass manager.
/// It provides the pass-instrumentation callbacks that measure the pass
/// execution time. They collect timing info into individual timers as
-/// passes are being run. At the end of its life-time it prints the resulting
-/// timing report.
+/// passes are being run.
class TimePassesHandler {
/// Value of this type is capable of uniquely identifying pass invocations.
/// It is a pair of string Pass-Identifier (which for now is common
@@ -48,8 +47,10 @@ class TimePassesHandler {
using PassInvocationID = std::pair<StringRef, unsigned>;
/// Groups of timers for passes and analyses.
- TimerGroup PassTG;
- TimerGroup AnalysisTG;
+ TimerGroup &PassTG =
+ NamedRegionTimer::getNamedTimerGroup(PassGroupName, PassGroupDesc);
+ TimerGroup &AnalysisTG = NamedRegionTimer::getNamedTimerGroup(
+ AnalysisGroupName, AnalysisGroupDesc);
using TimerVector = llvm::SmallVector<std::unique_ptr<Timer>, 4>;
/// Map of timers for pass invocations
@@ -71,12 +72,15 @@ class TimePassesHandler {
bool PerRun;
public:
+ static constexpr StringRef PassGroupName = "pass";
+ static constexpr StringRef AnalysisGroupName = "analysis";
+ static constexpr StringRef PassGroupDesc = "Pass execution timing report";
+ static constexpr StringRef AnalysisGroupDesc =
+ "Analysis execution timing report";
+
TimePassesHandler();
TimePassesHandler(bool Enabled, bool PerRun = false);
- /// Destructor handles the print action if it has not been handled before.
- ~TimePassesHandler() { print(); }
-
/// Prints out timing information and then resets the timers.
void print();
diff --git a/llvm/include/llvm/Support/Timer.h b/llvm/include/llvm/Support/Timer.h
index abe30451dd2f2..5a5082b6718ed 100644
--- a/llvm/include/llvm/Support/Timer.h
+++ b/llvm/include/llvm/Support/Timer.h
@@ -169,6 +169,11 @@ struct NamedRegionTimer : public TimeRegion {
explicit NamedRegionTimer(StringRef Name, StringRef Description,
StringRef GroupName,
StringRef GroupDescription, bool Enabled = true);
+
+ // Create or get a TimerGroup stored in the same global map owned by
+ // NamedRegionTimer.
+ static TimerGroup &getNamedTimerGroup(StringRef GroupName,
+ StringRef GroupDescription);
};
/// The TimerGroup class is used to group together related timers into a single
diff --git a/llvm/lib/IR/PassTimingInfo.cpp b/llvm/lib/IR/PassTimingInfo.cpp
index 46db2c74a5c76..4e27086e97ac5 100644
--- a/llvm/lib/IR/PassTimingInfo.cpp
+++ b/llvm/lib/IR/PassTimingInfo.cpp
@@ -63,16 +63,9 @@ class PassTimingInfo {
private:
StringMap<unsigned> PassIDCountMap; ///< Map that counts instances of passes
DenseMap<PassInstanceID, std::unique_ptr<Timer>> TimingData; ///< timers for pass instances
- TimerGroup TG;
+ TimerGroup *PassTG = nullptr;
public:
- /// Default constructor for yet-inactive timeinfo.
- /// Use \p init() to activate it.
- PassTimingInfo();
-
- /// Print out timing information and release timers.
- ~PassTimingInfo();
-
/// Initializes the static \p TheTimeInfo member to a non-null value when
/// -time-passes is enabled. Leaves it null otherwise.
///
@@ -94,14 +87,6 @@ class PassTimingInfo {
static ManagedStatic<sys::SmartMutex<true>> TimingInfoMutex;
-PassTimingInfo::PassTimingInfo() : TG("pass", "Pass execution timing report") {}
-
-PassTimingInfo::~PassTimingInfo() {
- // Deleting the timers accumulates their info into the TG member.
- // Then TG member is (implicitly) deleted, actually printing the report.
- TimingData.clear();
-}
-
void PassTimingInfo::init() {
if (TheTimeInfo || !TimePassesIsEnabled)
return;
@@ -110,12 +95,16 @@ void PassTimingInfo::init() {
// This guarantees that the object will be constructed after static globals,
// thus it will be destroyed before them.
static ManagedStatic<PassTimingInfo> TTI;
+ if (!TTI->PassTG)
+ TTI->PassTG = &NamedRegionTimer::getNamedTimerGroup(
+ TimePassesHandler::PassGroupName, TimePassesHandler::PassGroupDesc);
TheTimeInfo = &*TTI;
}
/// Prints out timing information and then resets the timers.
void PassTimingInfo::print(raw_ostream *OutStream) {
- TG.print(OutStream ? *OutStream : *CreateInfoOutputFile(), true);
+ assert(PassTG && "PassTG is null, did you call PassTimingInfo::Init()?");
+ PassTG->print(OutStream ? *OutStream : *CreateInfoOutputFile(), true);
}
Timer *PassTimingInfo::newPassTimer(StringRef PassID, StringRef PassDesc) {
@@ -124,7 +113,8 @@ Timer *PassTimingInfo::newPassTimer(StringRef PassID, StringRef PassDesc) {
// Appending description with a pass-instance number for all but the first one
std::string PassDescNumbered =
num <= 1 ? PassDesc.str() : formatv("{0} #{1}", PassDesc, num).str();
- return new Timer(PassID, PassDescNumbered, TG);
+ assert(PassTG && "PassTG is null, did you call PassTimingInfo::Init()?");
+ return new Timer(PassID, PassDescNumbered, *PassTG);
}
Timer *PassTimingInfo::getPassTimer(Pass *P, PassInstanceID Pass) {
@@ -193,9 +183,7 @@ Timer &TimePassesHandler::getPassTimer(StringRef PassID, bool IsPass) {
}
TimePassesHandler::TimePassesHandler(bool Enabled, bool PerRun)
- : PassTG("pass", "Pass execution timing report"),
- AnalysisTG("analysis", "Analysis execution timing report"),
- Enabled(Enabled), PerRun(PerRun) {}
+ : Enabled(Enabled), PerRun(PerRun) {}
TimePassesHandler::TimePassesHandler()
: TimePassesHandler(TimePassesIsEnabled, TimePassesPerRun) {}
diff --git a/llvm/lib/Support/Timer.cpp b/llvm/lib/Support/Timer.cpp
index eca726828c697..69a1846fec29b 100644
--- a/llvm/lib/Support/Timer.cpp
+++ b/llvm/lib/Support/Timer.cpp
@@ -222,16 +222,28 @@ class Name2PairMap {
StringRef GroupDescription) {
sys::SmartScopedLock<true> L(timerLock());
- std::pair<TimerGroup*, Name2TimerMap> &GroupEntry = Map[GroupName];
-
- if (!GroupEntry.first)
- GroupEntry.first = new TimerGroup(GroupName, GroupDescription);
-
+ std::pair<TimerGroup *, Name2TimerMap> &GroupEntry =
+ getGroupEntry(GroupName, GroupDescription);
Timer &T = GroupEntry.second[Name];
if (!T.isInitialized())
T.init(Name, Description, *GroupEntry.first);
return T;
}
+
+ TimerGroup &getTimerGroup(StringRef GroupName, StringRef GroupDescription) {
+ sys::SmartScopedLock<true> L(timerLock());
+ return *getGroupEntry(GroupName, GroupDescription).first;
+ }
+
+private:
+ std::pair<TimerGroup *, Name2TimerMap> &
+ getGroupEntry(StringRef GroupName, StringRef GroupDescription) {
+ std::pair<TimerGroup *, Name2TimerMap> &GroupEntry = Map[GroupName];
+ if (!GroupEntry.first)
+ GroupEntry.first = new TimerGroup(GroupName, GroupDescription);
+
+ return GroupEntry;
+ }
};
}
@@ -244,6 +256,11 @@ NamedRegionTimer::NamedRegionTimer(StringRef Name, StringRef Description,
: &namedGroupedTimers().get(Name, Description, GroupName,
GroupDescription)) {}
+TimerGroup &NamedRegionTimer::getNamedTimerGroup(StringRef GroupName,
+ StringRef GroupDescription) {
+ return namedGroupedTimers().getTimerGroup(GroupName, GroupDescription);
+}
+
//===----------------------------------------------------------------------===//
// TimerGroup Implementation
//===----------------------------------------------------------------------===//
diff --git a/llvm/unittests/IR/TimePassesTest.cpp b/llvm/unittests/IR/TimePassesTest.cpp
index 33f8e00b377d5..85986132103ca 100644
--- a/llvm/unittests/IR/TimePassesTest.cpp
+++ b/llvm/unittests/IR/TimePassesTest.cpp
@@ -161,8 +161,8 @@ TEST(TimePassesTest, CustomOut) {
PI.runBeforePass(Pass2, M);
PI.runAfterPass(Pass2, M, PreservedAnalyses::all());
- // Generate report by deleting the handler.
- TimePasses.reset();
+ // Clear and generate report again.
+ TimePasses->print();
// There should be Pass2 in this report and no Pass1.
EXPECT_FALSE(TimePassesStr.str().empty());
|
|
@llvm/pr-subscribers-llvm-ir Author: Alan Zhao (alanzhao1) ChangesThis reverts commit 31ebe66. The reason for the test failure is likely due to Full diff: https://github.com/llvm/llvm-project/pull/131217.diff 6 Files Affected:
diff --git a/clang/test/Misc/time-passes.c b/clang/test/Misc/time-passes.c
index 395da216aad42..c1669826b2268 100644
--- a/clang/test/Misc/time-passes.c
+++ b/clang/test/Misc/time-passes.c
@@ -19,6 +19,5 @@
// NPM: InstCombinePass{{$}}
// NPM-NOT: InstCombinePass #
// TIME: Total{{$}}
-// NPM: Pass execution timing report
int foo(int x, int y) { return x + y; }
diff --git a/llvm/include/llvm/IR/PassTimingInfo.h b/llvm/include/llvm/IR/PassTimingInfo.h
index 1148399943186..b47ba7f16ef37 100644
--- a/llvm/include/llvm/IR/PassTimingInfo.h
+++ b/llvm/include/llvm/IR/PassTimingInfo.h
@@ -39,8 +39,7 @@ Timer *getPassTimer(Pass *);
/// This class implements -time-passes functionality for new pass manager.
/// It provides the pass-instrumentation callbacks that measure the pass
/// execution time. They collect timing info into individual timers as
-/// passes are being run. At the end of its life-time it prints the resulting
-/// timing report.
+/// passes are being run.
class TimePassesHandler {
/// Value of this type is capable of uniquely identifying pass invocations.
/// It is a pair of string Pass-Identifier (which for now is common
@@ -48,8 +47,10 @@ class TimePassesHandler {
using PassInvocationID = std::pair<StringRef, unsigned>;
/// Groups of timers for passes and analyses.
- TimerGroup PassTG;
- TimerGroup AnalysisTG;
+ TimerGroup &PassTG =
+ NamedRegionTimer::getNamedTimerGroup(PassGroupName, PassGroupDesc);
+ TimerGroup &AnalysisTG = NamedRegionTimer::getNamedTimerGroup(
+ AnalysisGroupName, AnalysisGroupDesc);
using TimerVector = llvm::SmallVector<std::unique_ptr<Timer>, 4>;
/// Map of timers for pass invocations
@@ -71,12 +72,15 @@ class TimePassesHandler {
bool PerRun;
public:
+ static constexpr StringRef PassGroupName = "pass";
+ static constexpr StringRef AnalysisGroupName = "analysis";
+ static constexpr StringRef PassGroupDesc = "Pass execution timing report";
+ static constexpr StringRef AnalysisGroupDesc =
+ "Analysis execution timing report";
+
TimePassesHandler();
TimePassesHandler(bool Enabled, bool PerRun = false);
- /// Destructor handles the print action if it has not been handled before.
- ~TimePassesHandler() { print(); }
-
/// Prints out timing information and then resets the timers.
void print();
diff --git a/llvm/include/llvm/Support/Timer.h b/llvm/include/llvm/Support/Timer.h
index abe30451dd2f2..5a5082b6718ed 100644
--- a/llvm/include/llvm/Support/Timer.h
+++ b/llvm/include/llvm/Support/Timer.h
@@ -169,6 +169,11 @@ struct NamedRegionTimer : public TimeRegion {
explicit NamedRegionTimer(StringRef Name, StringRef Description,
StringRef GroupName,
StringRef GroupDescription, bool Enabled = true);
+
+ // Create or get a TimerGroup stored in the same global map owned by
+ // NamedRegionTimer.
+ static TimerGroup &getNamedTimerGroup(StringRef GroupName,
+ StringRef GroupDescription);
};
/// The TimerGroup class is used to group together related timers into a single
diff --git a/llvm/lib/IR/PassTimingInfo.cpp b/llvm/lib/IR/PassTimingInfo.cpp
index 46db2c74a5c76..4e27086e97ac5 100644
--- a/llvm/lib/IR/PassTimingInfo.cpp
+++ b/llvm/lib/IR/PassTimingInfo.cpp
@@ -63,16 +63,9 @@ class PassTimingInfo {
private:
StringMap<unsigned> PassIDCountMap; ///< Map that counts instances of passes
DenseMap<PassInstanceID, std::unique_ptr<Timer>> TimingData; ///< timers for pass instances
- TimerGroup TG;
+ TimerGroup *PassTG = nullptr;
public:
- /// Default constructor for yet-inactive timeinfo.
- /// Use \p init() to activate it.
- PassTimingInfo();
-
- /// Print out timing information and release timers.
- ~PassTimingInfo();
-
/// Initializes the static \p TheTimeInfo member to a non-null value when
/// -time-passes is enabled. Leaves it null otherwise.
///
@@ -94,14 +87,6 @@ class PassTimingInfo {
static ManagedStatic<sys::SmartMutex<true>> TimingInfoMutex;
-PassTimingInfo::PassTimingInfo() : TG("pass", "Pass execution timing report") {}
-
-PassTimingInfo::~PassTimingInfo() {
- // Deleting the timers accumulates their info into the TG member.
- // Then TG member is (implicitly) deleted, actually printing the report.
- TimingData.clear();
-}
-
void PassTimingInfo::init() {
if (TheTimeInfo || !TimePassesIsEnabled)
return;
@@ -110,12 +95,16 @@ void PassTimingInfo::init() {
// This guarantees that the object will be constructed after static globals,
// thus it will be destroyed before them.
static ManagedStatic<PassTimingInfo> TTI;
+ if (!TTI->PassTG)
+ TTI->PassTG = &NamedRegionTimer::getNamedTimerGroup(
+ TimePassesHandler::PassGroupName, TimePassesHandler::PassGroupDesc);
TheTimeInfo = &*TTI;
}
/// Prints out timing information and then resets the timers.
void PassTimingInfo::print(raw_ostream *OutStream) {
- TG.print(OutStream ? *OutStream : *CreateInfoOutputFile(), true);
+ assert(PassTG && "PassTG is null, did you call PassTimingInfo::Init()?");
+ PassTG->print(OutStream ? *OutStream : *CreateInfoOutputFile(), true);
}
Timer *PassTimingInfo::newPassTimer(StringRef PassID, StringRef PassDesc) {
@@ -124,7 +113,8 @@ Timer *PassTimingInfo::newPassTimer(StringRef PassID, StringRef PassDesc) {
// Appending description with a pass-instance number for all but the first one
std::string PassDescNumbered =
num <= 1 ? PassDesc.str() : formatv("{0} #{1}", PassDesc, num).str();
- return new Timer(PassID, PassDescNumbered, TG);
+ assert(PassTG && "PassTG is null, did you call PassTimingInfo::Init()?");
+ return new Timer(PassID, PassDescNumbered, *PassTG);
}
Timer *PassTimingInfo::getPassTimer(Pass *P, PassInstanceID Pass) {
@@ -193,9 +183,7 @@ Timer &TimePassesHandler::getPassTimer(StringRef PassID, bool IsPass) {
}
TimePassesHandler::TimePassesHandler(bool Enabled, bool PerRun)
- : PassTG("pass", "Pass execution timing report"),
- AnalysisTG("analysis", "Analysis execution timing report"),
- Enabled(Enabled), PerRun(PerRun) {}
+ : Enabled(Enabled), PerRun(PerRun) {}
TimePassesHandler::TimePassesHandler()
: TimePassesHandler(TimePassesIsEnabled, TimePassesPerRun) {}
diff --git a/llvm/lib/Support/Timer.cpp b/llvm/lib/Support/Timer.cpp
index eca726828c697..69a1846fec29b 100644
--- a/llvm/lib/Support/Timer.cpp
+++ b/llvm/lib/Support/Timer.cpp
@@ -222,16 +222,28 @@ class Name2PairMap {
StringRef GroupDescription) {
sys::SmartScopedLock<true> L(timerLock());
- std::pair<TimerGroup*, Name2TimerMap> &GroupEntry = Map[GroupName];
-
- if (!GroupEntry.first)
- GroupEntry.first = new TimerGroup(GroupName, GroupDescription);
-
+ std::pair<TimerGroup *, Name2TimerMap> &GroupEntry =
+ getGroupEntry(GroupName, GroupDescription);
Timer &T = GroupEntry.second[Name];
if (!T.isInitialized())
T.init(Name, Description, *GroupEntry.first);
return T;
}
+
+ TimerGroup &getTimerGroup(StringRef GroupName, StringRef GroupDescription) {
+ sys::SmartScopedLock<true> L(timerLock());
+ return *getGroupEntry(GroupName, GroupDescription).first;
+ }
+
+private:
+ std::pair<TimerGroup *, Name2TimerMap> &
+ getGroupEntry(StringRef GroupName, StringRef GroupDescription) {
+ std::pair<TimerGroup *, Name2TimerMap> &GroupEntry = Map[GroupName];
+ if (!GroupEntry.first)
+ GroupEntry.first = new TimerGroup(GroupName, GroupDescription);
+
+ return GroupEntry;
+ }
};
}
@@ -244,6 +256,11 @@ NamedRegionTimer::NamedRegionTimer(StringRef Name, StringRef Description,
: &namedGroupedTimers().get(Name, Description, GroupName,
GroupDescription)) {}
+TimerGroup &NamedRegionTimer::getNamedTimerGroup(StringRef GroupName,
+ StringRef GroupDescription) {
+ return namedGroupedTimers().getTimerGroup(GroupName, GroupDescription);
+}
+
//===----------------------------------------------------------------------===//
// TimerGroup Implementation
//===----------------------------------------------------------------------===//
diff --git a/llvm/unittests/IR/TimePassesTest.cpp b/llvm/unittests/IR/TimePassesTest.cpp
index 33f8e00b377d5..85986132103ca 100644
--- a/llvm/unittests/IR/TimePassesTest.cpp
+++ b/llvm/unittests/IR/TimePassesTest.cpp
@@ -161,8 +161,8 @@ TEST(TimePassesTest, CustomOut) {
PI.runBeforePass(Pass2, M);
PI.runAfterPass(Pass2, M, PreservedAnalyses::all());
- // Generate report by deleting the handler.
- TimePasses.reset();
+ // Clear and generate report again.
+ TimePasses->print();
// There should be Pass2 in this report and no Pass1.
EXPECT_FALSE(TimePassesStr.str().empty());
|
|
Let's wait until CI passes before merging as the test repro is kind of flaky on my machine. |
aeubanks
approved these changes
Mar 13, 2025
|
Looks like there's still some random bolt test failures going on. I think some test expectations need to be changed. |
Based on https://lab.llvm.org/buildbot/#/builders/153 these tests seem to be already broken on |
|
Buildkite is stuck on Linux-x64. But I ran the previously failing tests 100 times locally and I saw no failures, so it looks like this should be safe to merge. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/7456 Here is the relevant piece of the build log for the reference |
frederik-h
pushed a commit
to frederik-h/llvm-project
that referenced
this pull request
Mar 18, 2025
…s manager timers" (llvm#131173) (llvm#131217) This reverts commit 31ebe66. The reason for the test failure is likely due to `Name2PairMap::getTimerGroup(...)` not holding a lock.
alanzhao1
added a commit
to alanzhao1/llvm-project
that referenced
this pull request
Mar 19, 2025
After llvm#131217 was submitted, time-passes.ll fails because `opt` prints `-time-report` when `ManagedTimerGlobals` is destroyed. `ManagedTimerGlobals` stores `TimerGroup`s in an unordered map, so the ordering of the output `TimerGroup`s depends on the underlying iterator. To fix this, we do what Clang does and use `llvm::TimerGroup::printAll(...)`, which *is* deterministic. This is also what Clang does. This does put move analysis section before the pass section for `-time-report`, but again, this is also what Clang currently does.
alanzhao1
added a commit
that referenced
this pull request
Mar 27, 2025
…131941) After #131217 was submitted, time-passes.ll fails because `opt` prints `-time-report` when `ManagedTimerGlobals` is destroyed. `ManagedTimerGlobals` stores `TimerGroup`s in an unordered map, so the ordering of the output `TimerGroup`s depends on the underlying iterator. To fix this, we do what Clang does and use `llvm::TimerGroup::printAll(...)`, which *is* deterministic. This is also what Clang does. This does put move analysis section before the pass section for `-time-report`, but again, this is also what Clang currently does.
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Mar 27, 2025
…iterators (#131941) After llvm/llvm-project#131217 was submitted, time-passes.ll fails because `opt` prints `-time-report` when `ManagedTimerGlobals` is destroyed. `ManagedTimerGlobals` stores `TimerGroup`s in an unordered map, so the ordering of the output `TimerGroup`s depends on the underlying iterator. To fix this, we do what Clang does and use `llvm::TimerGroup::printAll(...)`, which *is* deterministic. This is also what Clang does. This does put move analysis section before the pass section for `-time-report`, but again, this is also what Clang currently does.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reverts commit 31ebe66.
The reason for the test failure is likely due to
Name2PairMap::getTimerGroup(...)not holding a lock.