[PATCH] D113857: [llvm-reduce] Add parallel chunk processing.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/tools/llvm-reduce/deltas/Delta.cpp:266
+
+    SmallVector<SmallString<0>> Results(NumJobs * 3, {});
+    for (auto I = ChunksStillConsideredInteresting.rbegin(),
----------------
Meinersbur wrote:
> [serious] `Results` is never resized to cover the last `NumChunksProcessed`, i.e. we get a buffer overflow. Instead, we could use a container that never invalidates its elements such as `std:deque` or use a circular buffer.
> 
> Alternatively, you can change `TaskQueue` to `std::dequeue<std::pair<std::shared_future<void>, ResultTy>>` to the task and its result in the same queue.
I think the code should only add new jobs if `NumChunkProcessed < Results.size()` in one iteration of the main loop, but there might be a an error. Adding it the to dequeue sounds like a great idea though to make things a bit simpler.

Alternatively we could also try to communicate the result through the `Future` directly, although that would require some changes to `ThreadPool`, because currently it just supports returning `shared_future<void>`.


================
Comment at: llvm/tools/llvm-reduce/deltas/Delta.cpp:306
+        //  * we have not reached the end of the chunk.
+        while (!TaskQueue.empty()) {
+          auto &F = TaskQueue.front();
----------------
Meinersbur wrote:
> This waits until the task queue is empty. Could we leave earlier as soon as a reduced version is available, and just forget the jobs not yet finished while scheduling the next ones?
The current approach waits for each task, until it finds the first one that leads to a reduction. 


It intentionally does not try to stop once any job reduces a chunk, because if we pick any reduced chunk we might get different results than during serial reduction.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113857/new/

https://reviews.llvm.org/D113857