From b2b4d522a442867aa65655cac1a0ece02616252b Mon Sep 17 00:00:00 2001 From: Alan Zhao Date: Mon, 24 Feb 2025 11:10:04 -0800 Subject: [PATCH 1/6] Use global TimerGroups for both new pass manager and old pass manager timers Additionally, remove the behavior for both pass manager's timer manager classes (`PassTimingInfo` for the old pass manager and `TimePassesHandler` for the new pass manager) where these classes would print the values of their timers upon destruction. Currently, each pass manager manages their own `TimerGroup`s. This is problematic because of duplicate `TimerGroup`s (both pass managers have a `TimerGroup` for pass times with identical names and descriptions). The result is that in Clang, `-ftime-report` has two "Pass execution timing report" sections (one for the new pass manager which manages optimization passes, and one for the old pass manager which manages the backend). The result of this change is that Clang's `-ftime-report` now prints both optimization and backend pass timing info in a unified "Pass execution timing report" section. Moving the ownership of the `TimerGroups` to globals also makes it easier to implement JSON-formatted `-ftime-report`. This was not possible with the old structure because the two pass managers were created and destroyed in far parts of the codebase and outputting JSON requires the printing logic to be at the same place because of formatting. Previous discourse discussion: https://discourse.llvm.org/t/difficulties-with-implementing-json-formatted-ftime-report/84353 --- clang/test/Misc/time-passes.c | 1 - llvm/include/llvm/IR/PassTimingInfo.h | 12 ++---- llvm/include/llvm/Support/Timer.h | 11 +++++ llvm/lib/IR/PassTimingInfo.cpp | 60 +++++++++++++-------------- llvm/lib/Support/Timer.cpp | 42 +++++++++++++++---- llvm/unittests/IR/TimePassesTest.cpp | 4 +- 6 files changed, 79 insertions(+), 51 deletions(-) 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..13947f7405245 100644 --- a/llvm/include/llvm/IR/PassTimingInfo.h +++ b/llvm/include/llvm/IR/PassTimingInfo.h @@ -39,19 +39,14 @@ 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 /// to all the instance of a given pass) + sequential invocation counter. using PassInvocationID = std::pair; - /// Groups of timers for passes and analyses. - TimerGroup PassTG; - TimerGroup AnalysisTG; - - using TimerVector = llvm::SmallVector, 4>; + using TimerVector = llvm::SmallVector; /// Map of timers for pass invocations StringMap TimingData; @@ -74,8 +69,7 @@ class TimePassesHandler { TimePassesHandler(); TimePassesHandler(bool Enabled, bool PerRun = false); - /// Destructor handles the print action if it has not been handled before. - ~TimePassesHandler() { print(); } + ~TimePassesHandler() = default; /// 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..3e115df1500fe 100644 --- a/llvm/include/llvm/Support/Timer.h +++ b/llvm/include/llvm/Support/Timer.h @@ -169,6 +169,17 @@ struct NamedRegionTimer : public TimeRegion { explicit NamedRegionTimer(StringRef Name, StringRef Description, StringRef GroupName, StringRef GroupDescription, bool Enabled = true); + + // Create or get a timer stored in the same global map as other timers owned + // by NamedRegionTimer. + static Timer &getNamedGroupTimer(StringRef Name, StringRef Description, + StringRef GroupName, + StringRef GroupDescription); + + // Create or get a TimerGroup stored in the same global map owned by + // NamedRegionTimer. + static TimerGroup &getNamedGroupTimerGroup(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..0d133be890ce2 100644 --- a/llvm/lib/IR/PassTimingInfo.cpp +++ b/llvm/lib/IR/PassTimingInfo.cpp @@ -37,6 +37,12 @@ namespace llvm { bool TimePassesIsEnabled = false; bool TimePassesPerRun = false; +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"; + static cl::opt EnableTiming( "time-passes", cl::location(TimePassesIsEnabled), cl::Hidden, cl::desc("Time each pass, printing elapsed time for each on exit")); @@ -62,16 +68,12 @@ class PassTimingInfo { private: StringMap PassIDCountMap; ///< Map that counts instances of passes - DenseMap> TimingData; ///< timers for pass instances - TimerGroup TG; + DenseMap TimingData; ///< timers for pass instances public: - /// Default constructor for yet-inactive timeinfo. - /// Use \p init() to activate it. - PassTimingInfo(); + PassTimingInfo() = default; - /// Print out timing information and release timers. - ~PassTimingInfo(); + ~PassTimingInfo() = default; /// Initializes the static \p TheTimeInfo member to a non-null value when /// -time-passes is enabled. Leaves it null otherwise. @@ -94,14 +96,6 @@ class PassTimingInfo { static ManagedStatic> 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; @@ -115,7 +109,8 @@ void PassTimingInfo::init() { /// Prints out timing information and then resets the timers. void PassTimingInfo::print(raw_ostream *OutStream) { - TG.print(OutStream ? *OutStream : *CreateInfoOutputFile(), true); + NamedRegionTimer::getNamedGroupTimerGroup(PassGroupName, PassGroupDesc) + .print(OutStream ? *OutStream : *CreateInfoOutputFile(), true); } Timer *PassTimingInfo::newPassTimer(StringRef PassID, StringRef PassDesc) { @@ -124,7 +119,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); + return &NamedRegionTimer::getNamedGroupTimer(PassID, PassDescNumbered, + PassGroupName, PassGroupDesc); } Timer *PassTimingInfo::getPassTimer(Pass *P, PassInstanceID Pass) { @@ -133,16 +129,16 @@ Timer *PassTimingInfo::getPassTimer(Pass *P, PassInstanceID Pass) { init(); sys::SmartScopedLock Lock(*TimingInfoMutex); - std::unique_ptr &T = TimingData[Pass]; + Timer *&T = TimingData[Pass]; if (!T) { StringRef PassName = P->getPassName(); StringRef PassArgument; if (const PassInfo *PI = Pass::lookupPassInfo(P->getPassID())) PassArgument = PI->getPassArgument(); - T.reset(newPassTimer(PassArgument.empty() ? PassName : PassArgument, PassName)); + T = newPassTimer(PassArgument.empty() ? PassName : PassArgument, PassName); } - return T.get(); + return T; } PassTimingInfo *PassTimingInfo::TheTimeInfo; @@ -170,11 +166,13 @@ void reportAndResetTimings(raw_ostream *OutStream) { /// Returns the timer for the specified pass invocation of \p PassID. /// Each time it creates a new timer. Timer &TimePassesHandler::getPassTimer(StringRef PassID, bool IsPass) { - TimerGroup &TG = IsPass ? PassTG : AnalysisTG; + StringRef TGName = IsPass ? PassGroupName : PassGroupDesc; + StringRef TGDesc = IsPass ? PassGroupDesc : AnalysisGroupDesc; + TimerGroup &TG = NamedRegionTimer::getNamedGroupTimerGroup(TGName, TGDesc); if (!PerRun) { TimerVector &Timers = TimingData[PassID]; if (Timers.size() == 0) - Timers.emplace_back(new Timer(PassID, PassID, TG)); + Timers.push_back(new Timer(PassID, PassID, TG)); return *Timers.front(); } @@ -186,16 +184,14 @@ Timer &TimePassesHandler::getPassTimer(StringRef PassID, bool IsPass) { std::string FullDesc = formatv("{0} #{1}", PassID, Count).str(); Timer *T = new Timer(PassID, FullDesc, TG); - Timers.emplace_back(T); + Timers.push_back(T); assert(Count == Timers.size() && "Timers vector not adjusted correctly."); return *T; } 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) {} @@ -215,8 +211,12 @@ void TimePassesHandler::print() { MaybeCreated = CreateInfoOutputFile(); OS = &*MaybeCreated; } - PassTG.print(*OS, true); - AnalysisTG.print(*OS, true); + + NamedRegionTimer::getNamedGroupTimerGroup(PassGroupName, PassGroupDesc) + .print(*OS, true); + NamedRegionTimer::getNamedGroupTimerGroup(AnalysisGroupName, + AnalysisGroupDesc) + .print(*OS, true); } LLVM_DUMP_METHOD void TimePassesHandler::dump() const { @@ -226,7 +226,7 @@ LLVM_DUMP_METHOD void TimePassesHandler::dump() const { StringRef PassID = I.getKey(); const TimerVector& MyTimers = I.getValue(); for (unsigned idx = 0; idx < MyTimers.size(); idx++) { - const Timer* MyTimer = MyTimers[idx].get(); + const Timer *MyTimer = MyTimers[idx]; if (MyTimer && MyTimer->isRunning()) dbgs() << "\tTimer " << MyTimer << " for pass " << PassID << "(" << idx << ")\n"; } @@ -236,7 +236,7 @@ LLVM_DUMP_METHOD void TimePassesHandler::dump() const { StringRef PassID = I.getKey(); const TimerVector& MyTimers = I.getValue(); for (unsigned idx = 0; idx < MyTimers.size(); idx++) { - const Timer* MyTimer = MyTimers[idx].get(); + const Timer *MyTimer = MyTimers[idx]; if (MyTimer && MyTimer->hasTriggered() && !MyTimer->isRunning()) dbgs() << "\tTimer " << MyTimer << " for pass " << PassID << "(" << idx << ")\n"; } diff --git a/llvm/lib/Support/Timer.cpp b/llvm/lib/Support/Timer.cpp index 089dae2886f22..0be1e5b47bee8 100644 --- a/llvm/lib/Support/Timer.cpp +++ b/llvm/lib/Support/Timer.cpp @@ -222,16 +222,27 @@ class Name2PairMap { StringRef GroupDescription) { sys::SmartScopedLock L(timerLock()); - std::pair &GroupEntry = Map[GroupName]; - - if (!GroupEntry.first) - GroupEntry.first = new TimerGroup(GroupName, GroupDescription); - + std::pair &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) { + return *getGroupEntry(GroupName, GroupDescription).first; + } + +private: + std::pair & + getGroupEntry(StringRef GroupName, StringRef GroupDescription) { + std::pair &GroupEntry = Map[GroupName]; + if (!GroupEntry.first) + GroupEntry.first = new TimerGroup(GroupName, GroupDescription); + + return GroupEntry; + } }; } @@ -239,10 +250,23 @@ class Name2PairMap { NamedRegionTimer::NamedRegionTimer(StringRef Name, StringRef Description, StringRef GroupName, StringRef GroupDescription, bool Enabled) - : TimeRegion(!Enabled - ? nullptr - : &namedGroupedTimers().get(Name, Description, GroupName, - GroupDescription)) {} + : TimeRegion(!Enabled ? nullptr + : &getNamedGroupTimer(Name, Description, GroupName, + GroupDescription)) {} + +Timer &NamedRegionTimer::getNamedGroupTimer(StringRef Name, + StringRef Description, + StringRef GroupName, + StringRef GroupDescription) { + return namedGroupedTimers().get(Name, Description, GroupName, + GroupDescription); +} + +TimerGroup & +NamedRegionTimer::getNamedGroupTimerGroup(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()); From 2c8eb5808d383d678e4e29ab30395284eb47bfb4 Mon Sep 17 00:00:00 2001 From: Alan Zhao Date: Mon, 10 Mar 2025 13:39:24 -0700 Subject: [PATCH 2/6] code review comments --- llvm/include/llvm/IR/PassTimingInfo.h | 11 +++++++++++ llvm/include/llvm/Support/Timer.h | 4 ++-- llvm/lib/IR/PassTimingInfo.cpp | 24 +++++++++--------------- llvm/lib/Support/Timer.cpp | 20 ++++++-------------- 4 files changed, 28 insertions(+), 31 deletions(-) diff --git a/llvm/include/llvm/IR/PassTimingInfo.h b/llvm/include/llvm/IR/PassTimingInfo.h index 13947f7405245..0d759c5c50ef8 100644 --- a/llvm/include/llvm/IR/PassTimingInfo.h +++ b/llvm/include/llvm/IR/PassTimingInfo.h @@ -65,7 +65,18 @@ class TimePassesHandler { bool Enabled; bool PerRun; + TimerGroup &PassTG = + NamedRegionTimer::getNamedTimerGroup(PassGroupName, PassGroupDesc); + TimerGroup &AnalysisTG = NamedRegionTimer::getNamedTimerGroup( + AnalysisGroupName, AnalysisGroupDesc); + 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); diff --git a/llvm/include/llvm/Support/Timer.h b/llvm/include/llvm/Support/Timer.h index 3e115df1500fe..5e2ac00b41358 100644 --- a/llvm/include/llvm/Support/Timer.h +++ b/llvm/include/llvm/Support/Timer.h @@ -178,8 +178,8 @@ struct NamedRegionTimer : public TimeRegion { // Create or get a TimerGroup stored in the same global map owned by // NamedRegionTimer. - static TimerGroup &getNamedGroupTimerGroup(StringRef GroupName, - StringRef GroupDescription); + 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 0d133be890ce2..405e7f7c4cd07 100644 --- a/llvm/lib/IR/PassTimingInfo.cpp +++ b/llvm/lib/IR/PassTimingInfo.cpp @@ -37,12 +37,6 @@ namespace llvm { bool TimePassesIsEnabled = false; bool TimePassesPerRun = false; -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"; - static cl::opt EnableTiming( "time-passes", cl::location(TimePassesIsEnabled), cl::Hidden, cl::desc("Time each pass, printing elapsed time for each on exit")); @@ -109,7 +103,8 @@ void PassTimingInfo::init() { /// Prints out timing information and then resets the timers. void PassTimingInfo::print(raw_ostream *OutStream) { - NamedRegionTimer::getNamedGroupTimerGroup(PassGroupName, PassGroupDesc) + NamedRegionTimer::getNamedTimerGroup(TimePassesHandler::PassGroupName, + TimePassesHandler::PassGroupDesc) .print(OutStream ? *OutStream : *CreateInfoOutputFile(), true); } @@ -119,8 +114,10 @@ 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 &NamedRegionTimer::getNamedGroupTimer(PassID, PassDescNumbered, - PassGroupName, PassGroupDesc); + return new Timer( + PassID, PassDescNumbered, + NamedRegionTimer::getNamedTimerGroup(TimePassesHandler::PassGroupName, + TimePassesHandler::PassGroupDesc)); } Timer *PassTimingInfo::getPassTimer(Pass *P, PassInstanceID Pass) { @@ -166,9 +163,7 @@ void reportAndResetTimings(raw_ostream *OutStream) { /// Returns the timer for the specified pass invocation of \p PassID. /// Each time it creates a new timer. Timer &TimePassesHandler::getPassTimer(StringRef PassID, bool IsPass) { - StringRef TGName = IsPass ? PassGroupName : PassGroupDesc; - StringRef TGDesc = IsPass ? PassGroupDesc : AnalysisGroupDesc; - TimerGroup &TG = NamedRegionTimer::getNamedGroupTimerGroup(TGName, TGDesc); + TimerGroup &TG = IsPass ? PassTG : AnalysisTG; if (!PerRun) { TimerVector &Timers = TimingData[PassID]; if (Timers.size() == 0) @@ -212,10 +207,9 @@ void TimePassesHandler::print() { OS = &*MaybeCreated; } - NamedRegionTimer::getNamedGroupTimerGroup(PassGroupName, PassGroupDesc) + NamedRegionTimer::getNamedTimerGroup(PassGroupName, PassGroupDesc) .print(*OS, true); - NamedRegionTimer::getNamedGroupTimerGroup(AnalysisGroupName, - AnalysisGroupDesc) + NamedRegionTimer::getNamedTimerGroup(AnalysisGroupName, AnalysisGroupDesc) .print(*OS, true); } diff --git a/llvm/lib/Support/Timer.cpp b/llvm/lib/Support/Timer.cpp index 0be1e5b47bee8..6a3f9fa32f1e9 100644 --- a/llvm/lib/Support/Timer.cpp +++ b/llvm/lib/Support/Timer.cpp @@ -250,21 +250,13 @@ class Name2PairMap { NamedRegionTimer::NamedRegionTimer(StringRef Name, StringRef Description, StringRef GroupName, StringRef GroupDescription, bool Enabled) - : TimeRegion(!Enabled ? nullptr - : &getNamedGroupTimer(Name, Description, GroupName, - GroupDescription)) {} + : TimeRegion(!Enabled + ? nullptr + : &namedGroupedTimers().get(Name, Description, GroupName, + GroupDescription)) {} -Timer &NamedRegionTimer::getNamedGroupTimer(StringRef Name, - StringRef Description, - StringRef GroupName, - StringRef GroupDescription) { - return namedGroupedTimers().get(Name, Description, GroupName, - GroupDescription); -} - -TimerGroup & -NamedRegionTimer::getNamedGroupTimerGroup(StringRef GroupName, - StringRef GroupDescription) { +TimerGroup &NamedRegionTimer::getNamedTimerGroup(StringRef GroupName, + StringRef GroupDescription) { return namedGroupedTimers().getTimerGroup(GroupName, GroupDescription); } From 6763fc3dd23695976a2fee2eeb8bdfdc2ed43707 Mon Sep 17 00:00:00 2001 From: Alan Zhao Date: Mon, 10 Mar 2025 13:46:44 -0700 Subject: [PATCH 3/6] move stuff around to keep the diffs smaller --- llvm/include/llvm/IR/PassTimingInfo.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/llvm/include/llvm/IR/PassTimingInfo.h b/llvm/include/llvm/IR/PassTimingInfo.h index 0d759c5c50ef8..3e3ea8b845b51 100644 --- a/llvm/include/llvm/IR/PassTimingInfo.h +++ b/llvm/include/llvm/IR/PassTimingInfo.h @@ -46,6 +46,12 @@ class TimePassesHandler { /// to all the instance of a given pass) + sequential invocation counter. using PassInvocationID = std::pair; + /// Groups of timers for passes and analyses. + TimerGroup &PassTG = + NamedRegionTimer::getNamedTimerGroup(PassGroupName, PassGroupDesc); + TimerGroup &AnalysisTG = NamedRegionTimer::getNamedTimerGroup( + AnalysisGroupName, AnalysisGroupDesc); + using TimerVector = llvm::SmallVector; /// Map of timers for pass invocations StringMap TimingData; @@ -65,11 +71,6 @@ class TimePassesHandler { bool Enabled; bool PerRun; - TimerGroup &PassTG = - NamedRegionTimer::getNamedTimerGroup(PassGroupName, PassGroupDesc); - TimerGroup &AnalysisTG = NamedRegionTimer::getNamedTimerGroup( - AnalysisGroupName, AnalysisGroupDesc); - public: static constexpr StringRef PassGroupName = "pass"; static constexpr StringRef AnalysisGroupName = "analysis"; From 7fcbfee0d4dcc85283af90022ccdcd09d48c917d Mon Sep 17 00:00:00 2001 From: Alan Zhao Date: Mon, 10 Mar 2025 14:10:54 -0700 Subject: [PATCH 4/6] more code review comments --- llvm/include/llvm/Support/Timer.h | 6 ------ llvm/lib/IR/PassTimingInfo.cpp | 21 ++++++++++----------- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/llvm/include/llvm/Support/Timer.h b/llvm/include/llvm/Support/Timer.h index 5e2ac00b41358..5a5082b6718ed 100644 --- a/llvm/include/llvm/Support/Timer.h +++ b/llvm/include/llvm/Support/Timer.h @@ -170,12 +170,6 @@ struct NamedRegionTimer : public TimeRegion { StringRef GroupName, StringRef GroupDescription, bool Enabled = true); - // Create or get a timer stored in the same global map as other timers owned - // by NamedRegionTimer. - static Timer &getNamedGroupTimer(StringRef Name, StringRef Description, - StringRef GroupName, - StringRef GroupDescription); - // Create or get a TimerGroup stored in the same global map owned by // NamedRegionTimer. static TimerGroup &getNamedTimerGroup(StringRef GroupName, diff --git a/llvm/lib/IR/PassTimingInfo.cpp b/llvm/lib/IR/PassTimingInfo.cpp index 405e7f7c4cd07..825297d7738f3 100644 --- a/llvm/lib/IR/PassTimingInfo.cpp +++ b/llvm/lib/IR/PassTimingInfo.cpp @@ -63,6 +63,7 @@ class PassTimingInfo { private: StringMap PassIDCountMap; ///< Map that counts instances of passes DenseMap TimingData; ///< timers for pass instances + TimerGroup *PassTG = nullptr; public: PassTimingInfo() = default; @@ -98,14 +99,16 @@ void PassTimingInfo::init() { // This guarantees that the object will be constructed after static globals, // thus it will be destroyed before them. static ManagedStatic 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) { - NamedRegionTimer::getNamedTimerGroup(TimePassesHandler::PassGroupName, - TimePassesHandler::PassGroupDesc) - .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) { @@ -114,10 +117,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, - NamedRegionTimer::getNamedTimerGroup(TimePassesHandler::PassGroupName, - TimePassesHandler::PassGroupDesc)); + assert(PassTG && "PassTG is null, did you call PassTimingInfo::Init()?"); + return new Timer(PassID, PassDescNumbered, *PassTG); } Timer *PassTimingInfo::getPassTimer(Pass *P, PassInstanceID Pass) { @@ -207,10 +208,8 @@ void TimePassesHandler::print() { OS = &*MaybeCreated; } - NamedRegionTimer::getNamedTimerGroup(PassGroupName, PassGroupDesc) - .print(*OS, true); - NamedRegionTimer::getNamedTimerGroup(AnalysisGroupName, AnalysisGroupDesc) - .print(*OS, true); + PassTG.print(*OS, true); + AnalysisTG.print(*OS, true); } LLVM_DUMP_METHOD void TimePassesHandler::dump() const { From cf7e631415e29d01b3758154215e95117fe18d2a Mon Sep 17 00:00:00 2001 From: Alan Zhao Date: Tue, 11 Mar 2025 15:03:09 -0700 Subject: [PATCH 5/6] yet more code review comments --- llvm/include/llvm/IR/PassTimingInfo.h | 2 -- llvm/lib/IR/PassTimingInfo.cpp | 12 ++++-------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/llvm/include/llvm/IR/PassTimingInfo.h b/llvm/include/llvm/IR/PassTimingInfo.h index 3e3ea8b845b51..6a61e63b47f10 100644 --- a/llvm/include/llvm/IR/PassTimingInfo.h +++ b/llvm/include/llvm/IR/PassTimingInfo.h @@ -81,8 +81,6 @@ class TimePassesHandler { TimePassesHandler(); TimePassesHandler(bool Enabled, bool PerRun = false); - ~TimePassesHandler() = default; - /// Prints out timing information and then resets the timers. void print(); diff --git a/llvm/lib/IR/PassTimingInfo.cpp b/llvm/lib/IR/PassTimingInfo.cpp index 825297d7738f3..f6fc1c78470f8 100644 --- a/llvm/lib/IR/PassTimingInfo.cpp +++ b/llvm/lib/IR/PassTimingInfo.cpp @@ -62,14 +62,10 @@ class PassTimingInfo { private: StringMap PassIDCountMap; ///< Map that counts instances of passes - DenseMap TimingData; ///< timers for pass instances + DenseMap> TimingData; ///< timers for pass instances TimerGroup *PassTG = nullptr; public: - PassTimingInfo() = default; - - ~PassTimingInfo() = default; - /// Initializes the static \p TheTimeInfo member to a non-null value when /// -time-passes is enabled. Leaves it null otherwise. /// @@ -127,16 +123,16 @@ Timer *PassTimingInfo::getPassTimer(Pass *P, PassInstanceID Pass) { init(); sys::SmartScopedLock Lock(*TimingInfoMutex); - Timer *&T = TimingData[Pass]; + std::unique_ptr &T = TimingData[Pass]; if (!T) { StringRef PassName = P->getPassName(); StringRef PassArgument; if (const PassInfo *PI = Pass::lookupPassInfo(P->getPassID())) PassArgument = PI->getPassArgument(); - T = newPassTimer(PassArgument.empty() ? PassName : PassArgument, PassName); + T.reset(newPassTimer(PassArgument.empty() ? PassName : PassArgument, PassName)); } - return T; + return T.get(); } PassTimingInfo *PassTimingInfo::TheTimeInfo; From b114409da3cfe636db4231120fd0bd673b9ee483 Mon Sep 17 00:00:00 2001 From: Alan Zhao Date: Wed, 12 Mar 2025 17:05:11 -0700 Subject: [PATCH 6/6] make TimePassesHandler own its Timers --- llvm/include/llvm/IR/PassTimingInfo.h | 2 +- llvm/lib/IR/PassTimingInfo.cpp | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/llvm/include/llvm/IR/PassTimingInfo.h b/llvm/include/llvm/IR/PassTimingInfo.h index 6a61e63b47f10..b47ba7f16ef37 100644 --- a/llvm/include/llvm/IR/PassTimingInfo.h +++ b/llvm/include/llvm/IR/PassTimingInfo.h @@ -52,7 +52,7 @@ class TimePassesHandler { TimerGroup &AnalysisTG = NamedRegionTimer::getNamedTimerGroup( AnalysisGroupName, AnalysisGroupDesc); - using TimerVector = llvm::SmallVector; + using TimerVector = llvm::SmallVector, 4>; /// Map of timers for pass invocations StringMap TimingData; diff --git a/llvm/lib/IR/PassTimingInfo.cpp b/llvm/lib/IR/PassTimingInfo.cpp index f6fc1c78470f8..4e27086e97ac5 100644 --- a/llvm/lib/IR/PassTimingInfo.cpp +++ b/llvm/lib/IR/PassTimingInfo.cpp @@ -164,7 +164,7 @@ Timer &TimePassesHandler::getPassTimer(StringRef PassID, bool IsPass) { if (!PerRun) { TimerVector &Timers = TimingData[PassID]; if (Timers.size() == 0) - Timers.push_back(new Timer(PassID, PassID, TG)); + Timers.emplace_back(new Timer(PassID, PassID, TG)); return *Timers.front(); } @@ -176,7 +176,7 @@ Timer &TimePassesHandler::getPassTimer(StringRef PassID, bool IsPass) { std::string FullDesc = formatv("{0} #{1}", PassID, Count).str(); Timer *T = new Timer(PassID, FullDesc, TG); - Timers.push_back(T); + Timers.emplace_back(T); assert(Count == Timers.size() && "Timers vector not adjusted correctly."); return *T; @@ -203,7 +203,6 @@ void TimePassesHandler::print() { MaybeCreated = CreateInfoOutputFile(); OS = &*MaybeCreated; } - PassTG.print(*OS, true); AnalysisTG.print(*OS, true); } @@ -215,7 +214,7 @@ LLVM_DUMP_METHOD void TimePassesHandler::dump() const { StringRef PassID = I.getKey(); const TimerVector& MyTimers = I.getValue(); for (unsigned idx = 0; idx < MyTimers.size(); idx++) { - const Timer *MyTimer = MyTimers[idx]; + const Timer* MyTimer = MyTimers[idx].get(); if (MyTimer && MyTimer->isRunning()) dbgs() << "\tTimer " << MyTimer << " for pass " << PassID << "(" << idx << ")\n"; } @@ -225,7 +224,7 @@ LLVM_DUMP_METHOD void TimePassesHandler::dump() const { StringRef PassID = I.getKey(); const TimerVector& MyTimers = I.getValue(); for (unsigned idx = 0; idx < MyTimers.size(); idx++) { - const Timer *MyTimer = MyTimers[idx]; + const Timer* MyTimer = MyTimers[idx].get(); if (MyTimer && MyTimer->hasTriggered() && !MyTimer->isRunning()) dbgs() << "\tTimer " << MyTimer << " for pass " << PassID << "(" << idx << ")\n"; }