[clang] [llvm] [llvm][NFC] Rework Timer.cpp globals to ensure valid lifetimes (PR #121663)
via llvm-commits
llvm-commits at lists.llvm.org
Sat Jan 4 12:34:31 PST 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-driver
Author: None (macurtis-amd)
<details>
<summary>Changes</summary>
This is intended to help with flang `-ftime-report` support:
- #<!-- -->107270.
With this change, I was able to cherry-pick #<!-- -->107270, uncomment `llvm::TimePassesIsEnabled = true;` and compile with `-ftime-report`.
I also noticed that `clang/lib/Driver/OffloadBundler.cpp` was statically constructing a `TimerGroup` and changed it to lazily construct via ManagedStatic.
---
Full diff: https://github.com/llvm/llvm-project/pull/121663.diff
3 Files Affected:
- (modified) clang/lib/Driver/OffloadBundler.cpp (+16-7)
- (modified) llvm/include/llvm/Support/Timer.h (+6)
- (modified) llvm/lib/Support/Timer.cpp (+160-88)
``````````diff
diff --git a/clang/lib/Driver/OffloadBundler.cpp b/clang/lib/Driver/OffloadBundler.cpp
index 87d7303d938c90..2d6bdff0393be5 100644
--- a/clang/lib/Driver/OffloadBundler.cpp
+++ b/clang/lib/Driver/OffloadBundler.cpp
@@ -37,6 +37,7 @@
#include "llvm/Support/ErrorOr.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/MD5.h"
+#include "llvm/Support/ManagedStatic.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
@@ -63,9 +64,17 @@ using namespace llvm;
using namespace llvm::object;
using namespace clang;
-static llvm::TimerGroup
- ClangOffloadBundlerTimerGroup("Clang Offload Bundler Timer Group",
- "Timer group for clang offload bundler");
+namespace {
+struct CreateClangOffloadBundlerTimerGroup {
+ static void *call() {
+ return new TimerGroup("Clang Offload Bundler Timer Group",
+ "Timer group for clang offload bundler");
+ }
+};
+} // namespace
+static llvm::ManagedStatic<llvm::TimerGroup,
+ CreateClangOffloadBundlerTimerGroup>
+ ClangOffloadBundlerTimerGroup;
/// Magic string that marks the existence of offloading data.
#define OFFLOAD_BUNDLER_MAGIC_STR "__CLANG_OFFLOAD_BUNDLE__"
@@ -987,7 +996,7 @@ CompressedOffloadBundle::compress(llvm::compression::Params P,
"Compression not supported");
llvm::Timer HashTimer("Hash Calculation Timer", "Hash calculation time",
- ClangOffloadBundlerTimerGroup);
+ *ClangOffloadBundlerTimerGroup);
if (Verbose)
HashTimer.startTimer();
llvm::MD5 Hash;
@@ -1004,7 +1013,7 @@ CompressedOffloadBundle::compress(llvm::compression::Params P,
Input.getBuffer().size());
llvm::Timer CompressTimer("Compression Timer", "Compression time",
- ClangOffloadBundlerTimerGroup);
+ *ClangOffloadBundlerTimerGroup);
if (Verbose)
CompressTimer.startTimer();
llvm::compression::compress(P, BufferUint8, CompressedBuffer);
@@ -1119,7 +1128,7 @@ CompressedOffloadBundle::decompress(const llvm::MemoryBuffer &Input,
"Unknown compressing method");
llvm::Timer DecompressTimer("Decompression Timer", "Decompression time",
- ClangOffloadBundlerTimerGroup);
+ *ClangOffloadBundlerTimerGroup);
if (Verbose)
DecompressTimer.startTimer();
@@ -1141,7 +1150,7 @@ CompressedOffloadBundle::decompress(const llvm::MemoryBuffer &Input,
// Recalculate MD5 hash for integrity check
llvm::Timer HashRecalcTimer("Hash Recalculation Timer",
"Hash recalculation time",
- ClangOffloadBundlerTimerGroup);
+ *ClangOffloadBundlerTimerGroup);
HashRecalcTimer.startTimer();
llvm::MD5 Hash;
llvm::MD5::MD5Result Result;
diff --git a/llvm/include/llvm/Support/Timer.h b/llvm/include/llvm/Support/Timer.h
index 1a32832b6c6536..c05389332b8045 100644
--- a/llvm/include/llvm/Support/Timer.h
+++ b/llvm/include/llvm/Support/Timer.h
@@ -12,6 +12,7 @@
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/DataTypes.h"
+#include "llvm/Support/Mutex.h"
#include <cassert>
#include <memory>
#include <string>
@@ -19,6 +20,7 @@
namespace llvm {
+class TimerGlobals;
class TimerGroup;
class raw_ostream;
@@ -196,6 +198,10 @@ class TimerGroup {
TimerGroup(const TimerGroup &TG) = delete;
void operator=(const TimerGroup &TG) = delete;
+ friend class TimerGlobals;
+ explicit TimerGroup(StringRef Name, StringRef Description,
+ sys::SmartMutex<true> &lock);
+
public:
explicit TimerGroup(StringRef Name, StringRef Description);
diff --git a/llvm/lib/Support/Timer.cpp b/llvm/lib/Support/Timer.cpp
index 634f27f57b00a2..5c0b5943d31f7d 100644
--- a/llvm/lib/Support/Timer.cpp
+++ b/llvm/lib/Support/Timer.cpp
@@ -38,63 +38,40 @@
using namespace llvm;
-// This ugly hack is brought to you courtesy of constructor/destructor ordering
-// being unspecified by C++. Basically the problem is that a Statistic object
-// gets destroyed, which ends up calling 'GetLibSupportInfoOutputFile()'
-// (below), which calls this function. LibSupportInfoOutputFilename used to be
-// a global variable, but sometimes it would get destroyed before the Statistic,
-// causing havoc to ensue. We "fix" this by creating the string the first time
-// it is needed and never destroying it.
-static ManagedStatic<std::string> LibSupportInfoOutputFilename;
-static std::string &getLibSupportInfoOutputFilename() {
- return *LibSupportInfoOutputFilename;
+//===----------------------------------------------------------------------===//
+// Forward declarations for Managed Timer Globals (mtg) getters.
+//
+// Globals have been placed at the end of the file to restrict direct
+// access. Use of getters also has the benefit of making it a bit more explicit
+// that a global is being used.
+//===----------------------------------------------------------------------===//
+namespace {
+class Name2PairMap;
}
-static ManagedStatic<sys::SmartMutex<true> > TimerLock;
-
-/// Allows llvm::Timer to emit signposts when supported.
-static ManagedStatic<SignpostEmitter> Signposts;
-
-namespace {
-struct CreateTrackSpace {
- static void *call() {
- return new cl::opt<bool>("track-memory",
- cl::desc("Enable -time-passes memory "
- "tracking (this may be slow)"),
- cl::Hidden);
- }
-};
-static ManagedStatic<cl::opt<bool>, CreateTrackSpace> TrackSpace;
-struct CreateInfoOutputFilename {
- static void *call() {
- return new cl::opt<std::string, true>(
- "info-output-file", cl::value_desc("filename"),
- cl::desc("File to append -stats and -timer output to"), cl::Hidden,
- cl::location(getLibSupportInfoOutputFilename()));
- }
-};
-static ManagedStatic<cl::opt<std::string, true>, CreateInfoOutputFilename>
- InfoOutputFilename;
-struct CreateSortTimers {
- static void *call() {
- return new cl::opt<bool>(
- "sort-timers",
- cl::desc("In the report, sort the timers in each group "
- "in wall clock time order"),
- cl::init(true), cl::Hidden);
- }
-};
-ManagedStatic<cl::opt<bool>, CreateSortTimers> SortTimers;
-} // namespace
+namespace mtg {
+static std::string &LibSupportInfoOutputFilename();
+static const std::string &InfoOutputFilename();
+static bool TrackSpace();
+static bool SortTimers();
+static SignpostEmitter &Signposts();
+static sys::SmartMutex<true> &TimerLock();
+static TimerGroup &DefaultTimerGroup();
+static TimerGroup *claimDefaultTimerGroup();
+static Name2PairMap &NamedGroupedTimers();
+} // namespace mtg
+//===----------------------------------------------------------------------===//
+//
+//===----------------------------------------------------------------------===//
void llvm::initTimerOptions() {
- *TrackSpace;
- *InfoOutputFilename;
- *SortTimers;
+ mtg::TrackSpace();
+ mtg::InfoOutputFilename();
+ mtg::SortTimers();
}
std::unique_ptr<raw_ostream> llvm::CreateInfoOutputFile() {
- const std::string &OutputFilename = getLibSupportInfoOutputFilename();
+ const std::string &OutputFilename = mtg::LibSupportInfoOutputFilename();
if (OutputFilename.empty())
return std::make_unique<raw_fd_ostream>(2, false); // stderr.
if (OutputFilename == "-")
@@ -115,22 +92,12 @@ std::unique_ptr<raw_ostream> llvm::CreateInfoOutputFile() {
return std::make_unique<raw_fd_ostream>(2, false); // stderr.
}
-namespace {
-struct CreateDefaultTimerGroup {
- static void *call() {
- return new TimerGroup("misc", "Miscellaneous Ungrouped Timers");
- }
-};
-} // namespace
-static ManagedStatic<TimerGroup, CreateDefaultTimerGroup> DefaultTimerGroup;
-static TimerGroup *getDefaultTimerGroup() { return &*DefaultTimerGroup; }
-
//===----------------------------------------------------------------------===//
// Timer Implementation
//===----------------------------------------------------------------------===//
void Timer::init(StringRef TimerName, StringRef TimerDescription) {
- init(TimerName, TimerDescription, *getDefaultTimerGroup());
+ init(TimerName, TimerDescription, mtg::DefaultTimerGroup());
}
void Timer::init(StringRef TimerName, StringRef TimerDescription,
@@ -149,7 +116,7 @@ Timer::~Timer() {
}
static inline size_t getMemUsage() {
- if (!*TrackSpace)
+ if (!mtg::TrackSpace())
return 0;
return sys::Process::GetMallocUsage();
}
@@ -190,7 +157,7 @@ TimeRecord TimeRecord::getCurrentTime(bool Start) {
void Timer::startTimer() {
assert(!Running && "Cannot start a running timer");
Running = Triggered = true;
- Signposts->startInterval(this, getName());
+ mtg::Signposts().startInterval(this, getName());
StartTime = TimeRecord::getCurrentTime(true);
}
@@ -199,7 +166,7 @@ void Timer::stopTimer() {
Running = false;
Time += TimeRecord::getCurrentTime(false);
Time -= StartTime;
- Signposts->endInterval(this, getName());
+ mtg::Signposts().endInterval(this, getName());
}
void Timer::clear() {
@@ -251,7 +218,7 @@ class Name2PairMap {
Timer &get(StringRef Name, StringRef Description, StringRef GroupName,
StringRef GroupDescription) {
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
std::pair<TimerGroup*, Name2TimerMap> &GroupEntry = Map[GroupName];
@@ -267,14 +234,13 @@ class Name2PairMap {
}
-static ManagedStatic<Name2PairMap> NamedGroupedTimers;
-
NamedRegionTimer::NamedRegionTimer(StringRef Name, StringRef Description,
StringRef GroupName,
StringRef GroupDescription, bool Enabled)
- : TimeRegion(!Enabled ? nullptr
- : &NamedGroupedTimers->get(Name, Description, GroupName,
- GroupDescription)) {}
+ : TimeRegion(!Enabled ? nullptr
+ : &mtg::NamedGroupedTimers().get(Name, Description,
+ GroupName,
+ GroupDescription)) {}
//===----------------------------------------------------------------------===//
// TimerGroup Implementation
@@ -284,11 +250,12 @@ NamedRegionTimer::NamedRegionTimer(StringRef Name, StringRef Description,
/// ctor/dtor and is protected by the TimerLock lock.
static TimerGroup *TimerGroupList = nullptr;
-TimerGroup::TimerGroup(StringRef Name, StringRef Description)
- : Name(Name.begin(), Name.end()),
- Description(Description.begin(), Description.end()) {
+TimerGroup::TimerGroup(StringRef Name, StringRef Description,
+ sys::SmartMutex<true> &lock)
+ : Name(Name.begin(), Name.end()),
+ Description(Description.begin(), Description.end()) {
// Add the group to TimerGroupList.
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(lock);
if (TimerGroupList)
TimerGroupList->Prev = &Next;
Next = TimerGroupList;
@@ -296,6 +263,9 @@ TimerGroup::TimerGroup(StringRef Name, StringRef Description)
TimerGroupList = this;
}
+TimerGroup::TimerGroup(StringRef Name, StringRef Description)
+ : TimerGroup(Name, Description, mtg::TimerLock()) {}
+
TimerGroup::TimerGroup(StringRef Name, StringRef Description,
const StringMap<TimeRecord> &Records)
: TimerGroup(Name, Description) {
@@ -313,7 +283,7 @@ TimerGroup::~TimerGroup() {
removeTimer(*FirstTimer);
// Remove the group from the TimerGroupList.
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
*Prev = Next;
if (Next)
Next->Prev = Prev;
@@ -321,7 +291,7 @@ TimerGroup::~TimerGroup() {
void TimerGroup::removeTimer(Timer &T) {
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
// If the timer was started, move its data to TimersToPrint.
if (T.hasTriggered())
@@ -344,7 +314,7 @@ void TimerGroup::removeTimer(Timer &T) {
}
void TimerGroup::addTimer(Timer &T) {
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
// Add the timer to our list.
if (FirstTimer)
@@ -356,7 +326,7 @@ void TimerGroup::addTimer(Timer &T) {
void TimerGroup::PrintQueuedTimers(raw_ostream &OS) {
// Perhaps sort the timers in descending order by amount of time taken.
- if (*SortTimers)
+ if (mtg::SortTimers())
llvm::sort(TimersToPrint);
TimeRecord Total;
@@ -374,7 +344,7 @@ void TimerGroup::PrintQueuedTimers(raw_ostream &OS) {
// If this is not an collection of ungrouped times, print the total time.
// Ungrouped timers don't really make sense to add up. We still print the
// TOTAL line to make the percentages make sense.
- if (this != getDefaultTimerGroup())
+ if (this != &mtg::DefaultTimerGroup())
OS << format(" Total Execution Time: %5.4f seconds (%5.4f wall clock)\n",
Total.getProcessTime(), Total.getWallTime());
OS << '\n';
@@ -426,7 +396,7 @@ void TimerGroup::prepareToPrintList(bool ResetTime) {
void TimerGroup::print(raw_ostream &OS, bool ResetAfterPrint) {
{
// After preparing the timers we can free the lock
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
prepareToPrintList(ResetAfterPrint);
}
@@ -436,20 +406,20 @@ void TimerGroup::print(raw_ostream &OS, bool ResetAfterPrint) {
}
void TimerGroup::clear() {
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
for (Timer *T = FirstTimer; T; T = T->Next)
T->clear();
}
void TimerGroup::printAll(raw_ostream &OS) {
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
for (TimerGroup *TG = TimerGroupList; TG; TG = TG->Next)
TG->print(OS);
}
void TimerGroup::clearAll() {
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
for (TimerGroup *TG = TimerGroupList; TG; TG = TG->Next)
TG->clear();
}
@@ -466,7 +436,7 @@ void TimerGroup::printJSONValue(raw_ostream &OS, const PrintRecord &R,
}
const char *TimerGroup::printJSONValues(raw_ostream &OS, const char *delim) {
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
prepareToPrintList(false);
for (const PrintRecord &R : TimersToPrint) {
@@ -493,17 +463,119 @@ const char *TimerGroup::printJSONValues(raw_ostream &OS, const char *delim) {
}
const char *TimerGroup::printAllJSONValues(raw_ostream &OS, const char *delim) {
- sys::SmartScopedLock<true> L(*TimerLock);
+ sys::SmartScopedLock<true> L(mtg::TimerLock());
for (TimerGroup *TG = TimerGroupList; TG; TG = TG->Next)
delim = TG->printJSONValues(OS, delim);
return delim;
}
void TimerGroup::constructForStatistics() {
- (void)getLibSupportInfoOutputFilename();
- (void)*NamedGroupedTimers;
+ mtg::LibSupportInfoOutputFilename();
+ mtg::NamedGroupedTimers();
}
std::unique_ptr<TimerGroup> TimerGroup::aquireDefaultGroup() {
- return std::unique_ptr<TimerGroup>(DefaultTimerGroup.claim());
+ return std::unique_ptr<TimerGroup>(mtg::claimDefaultTimerGroup());
+}
+
+//===----------------------------------------------------------------------===//
+// Timer Globals
+//
+// Previously, these were independent ManagedStatics. This led to bugs because
+// there are dependencies between the globals, but no reliable mechanism to
+// control relative lifetimes.
+//
+// Placing the globals within one class instance lets us control the lifetimes
+// of the various data members and ensure that no global uses another that has
+// been deleted.
+//
+// Globals fall into two categories. First are simple data types and
+// command-line options. These are cheap to construct and/or required early
+// during launch. They are created when the ManagedTimerGlobals singleton is
+// constructed. Second are types that are more expensive to construct or not
+// needed until later during compilation. These are lazily constructed in order
+// to reduce launch time.
+//===----------------------------------------------------------------------===//
+class llvm::TimerGlobals {
+public:
+ std::string LibSupportInfoOutputFilename;
+ cl::opt<std::string, true> InfoOutputFilename;
+ cl::opt<bool> TrackSpace;
+ cl::opt<bool> SortTimers;
+
+private:
+ // Order of these members and initialization below is important. For example
+ // the DefaultTimerGroup uses the TimerLock. Most of these also depend on the
+ // options above.
+ std::unique_ptr<SignpostEmitter> SignpostsPtr;
+ std::unique_ptr<sys::SmartMutex<true>> TimerLockPtr;
+ std::unique_ptr<TimerGroup> DefaultTimerGroupPtr;
+ std::unique_ptr<Name2PairMap> NamedGroupedTimersPtr;
+ std::once_flag InitDeferredFlag;
+ TimerGlobals &initDeferred() {
+ std::call_once(InitDeferredFlag, [this]() {
+ SignpostsPtr = std::make_unique<SignpostEmitter>();
+ TimerLockPtr = std::make_unique<sys::SmartMutex<true>>();
+ DefaultTimerGroupPtr.reset(new TimerGroup(
+ "misc", "Miscellaneous Ungrouped Timers", *TimerLockPtr));
+ NamedGroupedTimersPtr = std::make_unique<Name2PairMap>();
+ });
+ return *this;
+ }
+
+public:
+ SignpostEmitter &Signposts() { return *initDeferred().SignpostsPtr; }
+ sys::SmartMutex<true> &TimerLock() { return *initDeferred().TimerLockPtr; }
+ TimerGroup &DefaultTimerGroup() {
+ return *initDeferred().DefaultTimerGroupPtr;
+ }
+ TimerGroup *claimDefaultTimerGroup() {
+ return initDeferred().DefaultTimerGroupPtr.release();
+ }
+ Name2PairMap &NamedGroupedTimers() {
+ return *initDeferred().NamedGroupedTimersPtr;
+ }
+
+public:
+ TimerGlobals()
+ : InfoOutputFilename(
+ "info-output-file", cl::value_desc("filename"),
+ cl::desc("File to append -stats and -timer output to"), cl::Hidden,
+ cl::location(LibSupportInfoOutputFilename)),
+ TrackSpace(
+ "track-memory",
+ cl::desc("Enable -time-passes memory tracking (this may be slow)"),
+ cl::Hidden),
+ SortTimers(
+ "sort-timers",
+ cl::desc(
+ "In the report, sort the timers in each group in wall clock"
+ " time order"),
+ cl::init(true), cl::Hidden) {}
+};
+
+static ManagedStatic<TimerGlobals> ManagedTimerGlobals;
+
+static std::string &mtg::LibSupportInfoOutputFilename() {
+ return ManagedTimerGlobals->LibSupportInfoOutputFilename;
+}
+static const std::string &mtg::InfoOutputFilename() {
+ return ManagedTimerGlobals->InfoOutputFilename.getValue();
+}
+static bool mtg::TrackSpace() { return ManagedTimerGlobals->TrackSpace; };
+static bool mtg::SortTimers() { return ManagedTimerGlobals->SortTimers; }
+static SignpostEmitter &mtg::Signposts() {
+ return ManagedTimerGlobals->Signposts();
+}
+static sys::SmartMutex<true> &mtg::TimerLock() {
+ return ManagedTimerGlobals->TimerLock();
+}
+static TimerGroup &mtg::DefaultTimerGroup() {
+ return ManagedTimerGlobals->DefaultTimerGroup();
+}
+static TimerGroup *mtg::claimDefaultTimerGroup() {
+ return ManagedTimerGlobals->claimDefaultTimerGroup();
+}
+static Name2PairMap &mtg::NamedGroupedTimers() {
+ return ManagedTimerGlobals->NamedGroupedTimers();
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/121663
More information about the llvm-commits
mailing list