[Openmp-commits] [PATCH] D77609: [OpenMP] Added the support for unshackled task in RTL

[Alex via Phabricator via Openmp-commits]

adurang added a comment.

As a more general comment creating that many threads upfront even they're not necessary seems like a problematic thing (e.g., potential performance problems, tools confusion, ... ). Can we not delay the creation of the team to moment where is first necessary (i.e., at the first //target nowait region//?) This is a general principle followed through the RTL so far (whith sometimes an optional env var to advance that initialization).

Also, any plans on how this feature will be supported on Windows?

Last, are there any unit tests for this feature? I don't see any in the patch.

Thanks,



================
Comment at: openmp/runtime/src/kmp_tasking.cpp:965
+#endif
       __kmp_release_deps(gtid, taskdata);
     } else if (task_team && task_team->tt.tt_found_proxy_tasks) {
----------------
Calling release_deps from the unshackle thread will result in the tasks being released from the graph to be queued in the unshackle thread and not the original team which is wrong. You'd need to use here the original gtid but I'm not sure of the consequences of 'impersonating' a thread in that code ptah.


================
Comment at: openmp/runtime/src/kmp_tasking.cpp:1954
+    // task outside of any parallel region
+    must_wait = true;
+#endif
----------------
This statement here is going to create performance regressions **at least** in the serial path. It might probably affect the barrier performance of non-tasking applications which makes me very wary. You can use your unshackle_task counter to protected as is done a few lines before.


================
Comment at: openmp/runtime/src/kmp_tasking.cpp:3771
+  if (task_team)
+    while (KMP_ATOMIC_LD_ACQ(&task_team->tt.tt_unfinished_unshackled_tasks))
+      ;
----------------
It seems to me that this is forcing any taskwait (and possibly taskgroup) to wait for any outstanding "async target" that exist on the team irrespectively of being a part of that synchronization domain or not. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77609