[PATCH] D90639: Add parallelTransformReduce and parallelForEachError

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

More unit testing showed that I wasn't more than 1024 inputs well, so I fixed that. Take another look if you want to double check that. Thanks!



================
Comment at: llvm/include/llvm/Support/Parallel.h:169
+  // improving to take the number of available cores into account.)
+  size_t NumInputs = std::distance(Begin, End);
+  size_t NumTasks = std::min(static_cast<size_t>(1024), NumInputs);
----------------
rriddle wrote:
> Can you check for the case where `NumInputs == 0` and return Init? Any reason that we should require that there is always at least one task?
Let's just make it work. I think I hit this edge case somewhere in LLD for parallelForEach.


================
Comment at: llvm/include/llvm/Support/Parallel.h:172
+  size_t TaskSize = NumInputs / NumTasks;
+  assert(TaskSize > 0);
+
----------------
Hm, I guess this assert is wrong for empty inputs. Amusingly, LLVM hid the bug from me because integer divide by zero is UB. :)


================
Comment at: llvm/include/llvm/Support/Parallel.h:193
+  // reductions are cheaper than the transformation.
+  for (auto &R : Results)
+    Init = Reduce(Init, std::move(R));
----------------
rriddle wrote:
> We could strip the first reduce given that we know that there will be at least one task, but I suppose the reduce function won't be costly in most cases.
Eh, let's do it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90639