[LLD] Avoid non-deterministic relocations processing. #107186
Conversation
|
@llvm/pr-subscribers-lld-elf @llvm/pr-subscribers-llvm-support Author: Alexey Lapshin (avl-llvm) ChangesTaskGroup has a "sequential" mode for deterministic processing. LLD uses this for relocations handling. But, As it places relocations to different vectors(based on getThreadIndex), the actual locations can be non-deterministic: It is neccessary to use the same thread index to have the same locations. This patch changes "sequential" mode to always execute tasks on single thread with index 0. Fixes this #105958. Full diff: https://github.com/llvm/llvm-project/pull/107186.diff 3 Files Affected:
diff --git a/llvm/include/llvm/Support/Parallel.h b/llvm/include/llvm/Support/Parallel.h
index 8170da98f15a8c..0e1f4fadcf4fe6 100644
--- a/llvm/include/llvm/Support/Parallel.h
+++ b/llvm/include/llvm/Support/Parallel.h
@@ -96,9 +96,8 @@ 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 different
- // threads, but strictly in sequential order.
+ // exactly in the order which they were spawned and
+ // on the thread with index 0.
void spawn(std::function<void()> f, bool Sequential = false);
void sync() const { L.sync(); }
diff --git a/llvm/lib/Support/Parallel.cpp b/llvm/lib/Support/Parallel.cpp
index a3ef3d9c621b98..3ef3b65153937a 100644
--- a/llvm/lib/Support/Parallel.cpp
+++ b/llvm/lib/Support/Parallel.cpp
@@ -113,7 +113,7 @@ class ThreadPoolExecutor : public Executor {
private:
bool hasSequentialTasks() const {
- return !WorkQueueSequential.empty() && !SequentialQueueIsLocked;
+ return !WorkQueueSequential.empty() && (getThreadIndex() == 0);
}
bool hasGeneralTasks() const { return !WorkQueue.empty(); }
@@ -128,24 +128,15 @@ class ThreadPoolExecutor : public Executor {
});
if (Stop)
break;
- bool Sequential = hasSequentialTasks();
- if (Sequential)
- SequentialQueueIsLocked = true;
- else
- assert(hasGeneralTasks());
-
- auto &Queue = Sequential ? WorkQueueSequential : WorkQueue;
+ auto &Queue = hasSequentialTasks() ? WorkQueueSequential : WorkQueue;
auto Task = std::move(Queue.back());
Queue.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::mutex Mutex;
diff --git a/llvm/unittests/Support/ParallelTest.cpp b/llvm/unittests/Support/ParallelTest.cpp
index 0eafb9b401bee7..c6acd302945651 100644
--- a/llvm/unittests/Support/ParallelTest.cpp
+++ b/llvm/unittests/Support/ParallelTest.cpp
@@ -99,7 +99,12 @@ TEST(Parallel, TaskGroupSequentialFor) {
{
parallel::TaskGroup tg;
for (size_t Idx = 0; Idx < 500; Idx++)
- tg.spawn([&Count, Idx]() { EXPECT_EQ(Count++, Idx); }, true);
+ tg.spawn(
+ [&Count, Idx]() {
+ EXPECT_EQ(Count++, Idx);
+ EXPECT_EQ(llvm::parallel::getThreadIndex(), 0u);
+ },
+ true);
}
EXPECT_EQ(Count, 500ul);
}
|
llvm/lib/Support/Parallel.cpp
Outdated
| @@ -113,7 +113,7 @@ class ThreadPoolExecutor : public Executor { | |||
|
|
|||
| private: | |||
| bool hasSequentialTasks() const { | |||
| return !WorkQueueSequential.empty() && !SequentialQueueIsLocked; | |||
| return !WorkQueueSequential.empty() && (getThreadIndex() == 0); | |||
There was a problem hiding this comment.
IIUC, this change makes hasSequentialTasks() dependent on the thread index and hasSequentialTasks() is used as part of the Cond.wait() predicate in the worker thread main loop in work(). The add() method calls Cond.notify_one() to "wake" a single waiting worker thread. If the only outstanding task is sequential and the thread that is woken is not thread 0, then couldn't this possibly lead to the task never being executed? I think in practise because of "spurious wakeups", this would not happen but can that be guaranteed?
There was a problem hiding this comment.
excuse me for the delay. I am working on this issue.
Yes, we can change the implementation to guarantee that no tasks would be missed. Will submit changes.
| // exactly in the order which they were spawned. | ||
| // Note: Sequential tasks may be executed on different | ||
| // threads, but strictly in sequential order. | ||
| // exactly in the order which they were spawned and | ||
| // on the thread with index 0. |
There was a problem hiding this comment.
I remember this behaviour being slightly concerning when this functionality was added and now I know why...
There was a problem hiding this comment.
FTR, the discussion about the "concerning" behavior is this comment ("I don't think it is a problem as far as correctness, but might be somewhat unexpected.") on https://reviews.llvm.org/D148728
|
Shall we revert https://reviews.llvm.org/D148728 but possibly keep the The downside is that we need If needed, the assertion can be restored using a different mechanism:
|
I think we should essentially revert https://reviews.llvm.org/D148728 . Created #109084 |
TaskGroup has a "sequential" mode for deterministic processing.
LLD uses this for relocations handling. But, As it places relocations
to different vectors(based on getThreadIndex), the actual locations
can be non-deterministic:
if (shard)
part.relrDyn->relocsVec[parallel::getThreadIndex()].push_back(
{&isec, isec.relocs().size() - 1});
It is neccessary to use the same thread index to have the same locations.
This patch changes "sequential" mode to always execute tasks on single
thread with index 0.
Fixes this llvm#105958.
avl-llvm
force-pushed
the
avl-llvm/use-zero-thread-for-sequential
branch
from
d848092 to
b520876
Compare
|
This version of the patch is not finished. I uploaded it to show the possible solution. |
We can not temporarily set The assertion exactly prevent from the situation when we could have thread_index clashed. It seems that assertion has useful value. The alternative solution might be to have a separate thread and condition variable for the "sequential" tasks. In this way the thread indexes would not be clashed, "sequential" tasks would have the same order and thread index. I uploaded this possible solution, though it still not fully finished. |
Adding to the comment #109084 (comment) : I feel that maintaining the sequential mode in Parallel.cpp adds too much complexity ( My current feeling is that we should restore the original state before https://reviews.llvm.org/D148728 . |
|
closing in favor #109084 |
TaskGroup has a "sequential" mode for deterministic processing. LLD uses this for relocations handling. But, As it places relocations to different vectors(based on getThreadIndex), the actual locations can be non-deterministic:
It is neccessary to use the same thread index to have the same locations. This patch changes "sequential" mode to always execute tasks on single thread with index 0.
Fixes this #105958.