[llvm] b84d773 - [Parallel] Revert sequential task changes
Fangrui Song via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 20 21:15:47 PDT 2024
Author: Fangrui Song
Date: 2024-09-20T21:15:42-07:00
New Revision: b84d773fd004ce719da69fbae5ec1dbc2b951230
URL: https://github.com/llvm/llvm-project/commit/b84d773fd004ce719da69fbae5ec1dbc2b951230
DIFF: https://github.com/llvm/llvm-project/commit/b84d773fd004ce719da69fbae5ec1dbc2b951230.diff
LOG: [Parallel] Revert sequential task changes
https://reviews.llvm.org/D148728 introduced `bool Sequential` to unify
`execute` and the old `spawn` without argument. However, sequential
tasks might be executed by any worker thread (non-deterministic),
leading to non-determinism output for ld.lld -z nocombreloc (see
https://reviews.llvm.org/D133003).
In addition, the extra member variables have overhead.
This sequential task has only been used for lld parallel relocation
scanning.
This patch restores the behavior before https://reviews.llvm.org/D148728 .
Fix #105958
Pull Request: https://github.com/llvm/llvm-project/pull/109084
Added:
Modified:
lld/ELF/Relocations.cpp
llvm/include/llvm/Support/Parallel.h
llvm/lib/Support/Parallel.cpp
llvm/unittests/Support/ParallelTest.cpp
Removed:
################################################################################
diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index 6c07051a231537..e9227a6047d3ea 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -1647,30 +1647,44 @@ template <class ELFT> void elf::scanRelocations() {
bool serial = !config->zCombreloc || config->emachine == EM_MIPS ||
config->emachine == EM_PPC64;
parallel::TaskGroup tg;
- for (ELFFileBase *f : ctx.objectFiles) {
- auto fn = [f]() {
+ auto outerFn = [&]() {
+ for (ELFFileBase *f : ctx.objectFiles) {
+ auto fn = [f]() {
+ RelocationScanner scanner;
+ for (InputSectionBase *s : f->getSections()) {
+ if (s && s->kind() == SectionBase::Regular && s->isLive() &&
+ (s->flags & SHF_ALLOC) &&
+ !(s->type == SHT_ARM_EXIDX && config->emachine == EM_ARM))
+ scanner.template scanSection<ELFT>(*s);
+ }
+ };
+ if (serial)
+ fn();
+ else
+ tg.spawn(fn);
+ }
+ auto scanEH = [] {
RelocationScanner scanner;
- for (InputSectionBase *s : f->getSections()) {
- if (s && s->kind() == SectionBase::Regular && s->isLive() &&
- (s->flags & SHF_ALLOC) &&
- !(s->type == SHT_ARM_EXIDX && config->emachine == EM_ARM))
- scanner.template scanSection<ELFT>(*s);
+ for (Partition &part : ctx.partitions) {
+ for (EhInputSection *sec : part.ehFrame->sections)
+ scanner.template scanSection<ELFT>(*sec, /*isEH=*/true);
+ if (part.armExidx && part.armExidx->isLive())
+ for (InputSection *sec : part.armExidx->exidxSections)
+ if (sec->isLive())
+ scanner.template scanSection<ELFT>(*sec);
}
};
- tg.spawn(fn, serial);
- }
-
- tg.spawn([] {
- RelocationScanner scanner;
- for (Partition &part : ctx.partitions) {
- for (EhInputSection *sec : part.ehFrame->sections)
- scanner.template scanSection<ELFT>(*sec, /*isEH=*/true);
- if (part.armExidx && part.armExidx->isLive())
- for (InputSection *sec : part.armExidx->exidxSections)
- if (sec->isLive())
- scanner.template scanSection<ELFT>(*sec);
- }
- });
+ if (serial)
+ scanEH();
+ else
+ tg.spawn(scanEH);
+ };
+ // If `serial` is true, call `spawn` to ensure that `scanner` runs in a thread
+ // with valid getThreadIndex().
+ if (serial)
+ tg.spawn(outerFn);
+ else
+ outerFn();
}
static bool handleNonPreemptibleIfunc(Symbol &sym, uint16_t flags) {
diff --git a/llvm/include/llvm/Support/Parallel.h b/llvm/include/llvm/Support/Parallel.h
index 8170da98f15a8c..c34619ab7d96e6 100644
--- a/llvm/include/llvm/Support/Parallel.h
+++ b/llvm/include/llvm/Support/Parallel.h
@@ -97,9 +97,7 @@ class TaskGroup {
// Spawn a task, but does not wait for it to finish.
// Tasks marked with \p Sequential will be executed
// exactly in the order which they were spawned.
- // Note: Sequential tasks may be executed on
diff erent
- // threads, but strictly in sequential order.
- void spawn(std::function<void()> f, bool Sequential = false);
+ void spawn(std::function<void()> f);
void sync() const { L.sync(); }
diff --git a/llvm/lib/Support/Parallel.cpp b/llvm/lib/Support/Parallel.cpp
index a3ef3d9c621b98..2ba02b73dd8f16 100644
--- a/llvm/lib/Support/Parallel.cpp
+++ b/llvm/lib/Support/Parallel.cpp
@@ -12,7 +12,6 @@
#include "llvm/Support/Threading.h"
#include <atomic>
-#include <deque>
#include <future>
#include <thread>
#include <vector>
@@ -39,7 +38,7 @@ namespace {
class Executor {
public:
virtual ~Executor() = default;
- virtual void add(std::function<void()> func, bool Sequential = false) = 0;
+ virtual void add(std::function<void()> func) = 0;
virtual size_t getThreadCount() const = 0;
static Executor *getDefaultExecutor();
@@ -98,13 +97,10 @@ class ThreadPoolExecutor : public Executor {
static void call(void *Ptr) { ((ThreadPoolExecutor *)Ptr)->stop(); }
};
- void add(std::function<void()> F, bool Sequential = false) override {
+ void add(std::function<void()> F) override {
{
std::lock_guard<std::mutex> Lock(Mutex);
- if (Sequential)
- WorkQueueSequential.emplace_front(std::move(F));
- else
- WorkQueue.emplace_back(std::move(F));
+ WorkStack.push_back(std::move(F));
}
Cond.notify_one();
}
@@ -112,42 +108,23 @@ class ThreadPoolExecutor : public Executor {
size_t getThreadCount() const override { return ThreadCount; }
private:
- bool hasSequentialTasks() const {
- return !WorkQueueSequential.empty() && !SequentialQueueIsLocked;
- }
-
- bool hasGeneralTasks() const { return !WorkQueue.empty(); }
-
void work(ThreadPoolStrategy S, unsigned ThreadID) {
threadIndex = ThreadID;
S.apply_thread_strategy(ThreadID);
while (true) {
std::unique_lock<std::mutex> Lock(Mutex);
- Cond.wait(Lock, [&] {
- return Stop || hasGeneralTasks() || hasSequentialTasks();
- });
+ Cond.wait(Lock, [&] { return Stop || !WorkStack.empty(); });
if (Stop)
break;
- bool Sequential = hasSequentialTasks();
- if (Sequential)
- SequentialQueueIsLocked = true;
- else
- assert(hasGeneralTasks());
-
- auto &Queue = Sequential ? WorkQueueSequential : WorkQueue;
- auto Task = std::move(Queue.back());
- Queue.pop_back();
+ auto Task = std::move(WorkStack.back());
+ WorkStack.pop_back();
Lock.unlock();
Task();
- if (Sequential)
- SequentialQueueIsLocked = false;
}
}
std::atomic<bool> Stop{false};
- std::atomic<bool> SequentialQueueIsLocked{false};
- std::deque<std::function<void()>> WorkQueue;
- std::deque<std::function<void()>> WorkQueueSequential;
+ std::vector<std::function<void()>> WorkStack;
std::mutex Mutex;
std::condition_variable Cond;
std::promise<void> ThreadsCreated;
@@ -214,16 +191,14 @@ TaskGroup::~TaskGroup() {
L.sync();
}
-void TaskGroup::spawn(std::function<void()> F, bool Sequential) {
+void TaskGroup::spawn(std::function<void()> F) {
#if LLVM_ENABLE_THREADS
if (Parallel) {
L.inc();
- detail::Executor::getDefaultExecutor()->add(
- [&, F = std::move(F)] {
- F();
- L.dec();
- },
- Sequential);
+ detail::Executor::getDefaultExecutor()->add([&, F = std::move(F)] {
+ F();
+ L.dec();
+ });
return;
}
#endif
diff --git a/llvm/unittests/Support/ParallelTest.cpp b/llvm/unittests/Support/ParallelTest.cpp
index 0eafb9b401bee7..ccf5f2bb913244 100644
--- a/llvm/unittests/Support/ParallelTest.cpp
+++ b/llvm/unittests/Support/ParallelTest.cpp
@@ -94,16 +94,6 @@ TEST(Parallel, ForEachError) {
EXPECT_EQ(errText, std::string("asdf\nasdf\nasdf"));
}
-TEST(Parallel, TaskGroupSequentialFor) {
- size_t Count = 0;
- {
- parallel::TaskGroup tg;
- for (size_t Idx = 0; Idx < 500; Idx++)
- tg.spawn([&Count, Idx]() { EXPECT_EQ(Count++, Idx); }, true);
- }
- EXPECT_EQ(Count, 500ul);
-}
-
#if LLVM_ENABLE_THREADS
TEST(Parallel, NestedTaskGroup) {
// This test checks:
More information about the llvm-commits
mailing list