[Openmp-commits] [PATCH] D67447: Enable tasks dependencies hashmaps resizing

[Andrey Churbanov via Phabricator via Openmp-commits]

AndreyChurbanov accepted this revision.
AndreyChurbanov added inline comments.
This revision is now accepted and ready to land.
Herald added a subscriber: dmgreen.


================
Comment at: runtime/src/kmp_taskdeps.cpp:72
+  if (gen >= MAX_GEN)
+    return current_dephash;
+  size_t new_size = sizes[gen];
----------------
jdoerfert wrote:
> Do we want to give up here? I've seen people with *a lot* of dynamic tasks so we might want to scale somehow.
I don't think making unlimited hash is the right way to scale application with millions of different dependences per single hash.  There should be trivial workaround for users, like replace
```
    for (i = 0; i < N; i++) {
      #pragma omp task depend(inout: deps[i])
```
with
```
    for (i = 0; i < N; i++) {
      #pragma omp task depend(inout: deps[i%some_threshold])
```
This should work fine given that some_threshold is reasonably bigger than number of threads.  Rather than having unlimited hash, same dependences can better be re-used without losing semantics, e.g. if previous tasks had already been executed by the team. Even if this is not realistic, e.g. because tasks are executed too slowly comparing to speed of their generation, the introduced overhead of extra dependences should be negligible comparing to maintaining unlimited hash size (my view may be questionable of cause...).

I also thought of things like cleaning hash entry after the dependence is not needed any more, but his does not look simple to implement. And may impact performance because of needed extra synchronizations.


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D67447