[OpenMP] Enable profiling on multiple threads

[markdewing]

Summary

This patch fixes the issue where the built-in profiler doesn't produce data for multiple threads:
https://github.com/llvm/llvm-project/issues/57985

Before creating a TimeScope object, the code checks to see if a profiler instance has
been created for that thread. If not, it creates one and saves a pointer for later.
At shutdown, all the saved profiler instances are finished before the call to write out the data.

[markdewing]

This patch involves changes to the LLVM library and to OpenMP. I want one patch showing the LLVM library changes in context, though I can close this one and submit two separate patches if that would be better.

[jdoerfert]

You don't need to split it necessarily. Can you add people that worked on the time profile before as reviewers though.

[jdoerfert]

Isn't this duplicating the time profiler logic in libomptarget? I'm not sure if there is a better way but this seems unfortunate.

llvm/lib/Support/TimeProfiler.cpp

324 ↗

(On Diff #483671)

Nit: Prof, similar in other places.

openmp/libomptarget/src/rtl.cpp

47

Why is this a pointer? And why a std::vector not llvm::SmallVector

[markdewing]

openmp/libomptarget/src/rtl.cpp

47

If it is not a pointer, the following error happens

a.out: /home/mdewing/nvme/software/llvm/git/llvm-project/llvm/lib/Support/TimeProfiler.cpp:154: void llvm::TimeTraceProfiler::write(llvm::raw_pwrite_stream&): Assertion `llvm::all_of(Instances.List, [](const auto &TTP) { return TTP->Stack.empty(); }) && "All profiler sections should be ended when calling write"' failed.

This seems to be happening because the first entry in the vector is missing. I did not track down the root cause, but my guess is something to do with constructor ordering.

[jdoerfert]

This is only needed if you link OpenMP as static library, or always?

openmp/libomptarget/src/rtl.cpp

47

That doesn't make sense to me. What has an indirection here to do with the interaction with TimeProfiler.cpp.

[jdoerfert]

What if we add an instances vector argument to`timeTraceProfilerFinishThread`.
Our threads would work as before but when they finish they use the runtime global value "Instances" which was set in the constructor via getTimeTraceProfilerInstances.
Net benefit, assuming I got it right, is that we only need one extra argument, one global pointer, and to expose the function.
Afterwards, they should all agree on the instances vector and our problems should be gone, no?

[markdewing]

openmp/libomptarget/src/rtl.cpp

47

I looked into this more, and it does have to do with the ordering of destructors. The "deinit" function has the destructor attribute with a priority. The ordering of those destructors and the destructors of file-scope objects is not defined. And it looks like the destructor for file-scope objects get called first. So the memory for this vector gets freed before it's used in "deinit".

[markdewing]

Add comment. Change variable name.

openmp/libomptarget/src/rtl.cpp

static const char *RTLNames[] = {
     /* Remote target        */ "libomptarget.rtl.rpc",
 };
 
 PluginManager *PM;
 
 static char *ProfileTraceFile = nullptr;
 
 // List of TimeTraceProfiler instances on other threads
+// Needs to be pointer. If this is a file-scope object, the destructor
+// gets called before "deinit" is called.
 std::vector<llvm::TimeTraceProfiler *> *profileInstancesOtherThreads = nullptr;
 std::mutex profLock;
 
 __attribute__((constructor(101))) void init() {
   DP("Init target library!\n");
 
   bool UseEventsForAtomicTransfers = true;
   if (const char *ForceAtomicMap = getenv("LIBOMPTARGET_MAP_FORCE_ATOMIC")) {
__attribute__((constructor(101))) void init() {
   }
 }
 
 __attribute__((destructor(101))) void deinit() {
   DP("Deinit target library!\n");
   delete PM;
 
   if (ProfileTraceFile) {
-    for (auto prof : *profileInstancesOtherThreads)
-      timeTraceProfilerFinishThreadExternal(prof);
+    for (auto Prof : *profileInstancesOtherThreads)
+      timeTraceProfilerFinishThreadExternal(Prof);
     // TODO: add env var for file output
     if (auto E = timeTraceProfilerWrite(ProfileTraceFile, "-"))
       fprintf(stderr, "Error writing out the time trace\n");
 
     timeTraceProfilerCleanup();
     delete profileInstancesOtherThreads;
   }
 }
 
 // Check if time trace profiler is already initialized on this thread.
 // If not, initialize it and save a pointer to call finish before
 // writing out the file.
 void checkTimeTraceInitThisThread() {
   if (ProfileTraceFile) {
     if (llvm::getTimeTraceProfilerInstance() == nullptr) {
       DP("initializing profiler on thread");
       llvm::timeTraceProfilerInitialize(500, "libomptarget");
-      llvm::TimeTraceProfiler *prof = llvm::getTimeTraceProfilerInstance();
+      llvm::TimeTraceProfiler *Prof = llvm::getTimeTraceProfilerInstance();
       std::lock_guard<std::mutex> Lock(profLock);
-      profileInstancesOtherThreads->push_back(prof);
+      profileInstancesOtherThreads->push_back(Prof);
     }
   }
 }
 
 void RTLsTy::loadRTLs() {
   // Parse environment variable OMP_TARGET_OFFLOAD (if set)
   PM->TargetOffloadPolicy =
       (kmp_target_offload_kind_t)__kmpc_get_target_offload();