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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

I left some comments. Generally, I would prefer we minimize the use of the macro to elide declarations. I'd also prefer to use the macro as part of the conditions to avoid duplication.
Instead of

  #ifdef MACRO
  foo(X)
  #else 
  foo(Y)
  #endif

we do

  v = MACRO ? X : Y;
  foo(v);

which is really helpful if `foo` is complex code and just as fast.

@adurang @AndreyChurbanov have your concerns been addressed?



================
Comment at: openmp/runtime/src/kmp.h:2298
+  // encountering thread is never nullptr, therefore we it when this task is
+  // created.
+  kmp_task_team_t *td_parent_task_team;
----------------
Grammar:
"The task team of its parent task team"
and
"therefore we it when this task is created"


================
Comment at: openmp/runtime/src/kmp.h:2308
+
+#endif
   kmp_int32 td_size_alloc; // The size of task structure, including shareds etc.
----------------
Similarly, I don't think the byte savings here are worth it.


================
Comment at: openmp/runtime/src/kmp.h:2239
   unsigned detachable : 1; /* 1 == can detach */
-  unsigned reserved : 9; /* reserved for compiler use */
+#if USE_UNSHACKLED_TASK
+  unsigned unshackled : 1; /* 1 == unshackled task */
----------------
AndreyChurbanov wrote:
> This breaks the convention that the struct size is 32 bits.  The correct addition of extra flag under condition should look like:
> 
>   #if USE_UNSHACKLED_TASK
>     unsigned unshackled : 1; /* 1 == unshackled task */
>     unsigned reserved : 8; /* reserved for compiler use */
>   #else
>     unsigned reserved : 9; /* reserved for compiler use */
>   #endif
> 
Do we really need the `USE_UNSHACKLED_TASK` flag? Even if we want it, we don't need it here in the struct. Let's waste one bit on windows until we catch up and remove complexity for everyone.


================
Comment at: openmp/runtime/src/kmp_runtime.cpp:3675
   KMP_ASSERT(gtid < __kmp_threads_capacity);
+#endif
 
----------------
Is the code in the `#else` case the same as in the `} else {` case? If so, make the conditional `if (USE_UNSHACKLED_TASK && ...)` and avoid the duplication of bugs.


================
Comment at: openmp/runtime/src/kmp_runtime.cpp:4336
+                      ? 1
+                      : __kmp_unshackled_threads_num + 1;
+       TCR_PTR(__kmp_threads[new_gtid]) != NULL; ++new_gtid) {
----------------
Nit: can we move the initialization out of the loop, hard to read. A comment might help as well.

Looking at the code more generally, is this the same code as below with different bounds? If so, avoid the duplication all together please, same way as suggested above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77609