[Openmp-commits] [PATCH] D81989: [OpenMP] Introduce low level dependency process to target offloading

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added reviewers: grokos, AndreyChurbanov.
jdoerfert added a comment.

Two high level comments below. We need to split this patch.

Can you explain the approach in a bit more detail in the commit message? (Also a typo in there).



================
Comment at: openmp/libomptarget/src/omptarget.cpp:596
+           int32_t team_num, int32_t thread_limit, int IsTeamConstruct,
+           __tgt_async_info *AsyncInfo) {
   DeviceTy &Device = Devices[device_id];
----------------
I guess we can make async info a pointer argument in a separate (NFC) patch to reduce this one, WDYT?


================
Comment at: openmp/libomptarget/src/rtl.cpp:418
+                            1 /* team_num */, 1 /* thread_limit */,
+                            true /* IsTeamConstruct */, nullptr);
             if (rc != OFFLOAD_SUCCESS) {
----------------
Style: Everywhere I have seen this we do `/* name */ value`. I know this was different here but I'd like us to align with LLVM & Clang on this one.
Feel free to commit the comments for all but the new argument as NFC without further review.


================
Comment at: openmp/libomptarget/src/rtl.h:111
+  // Whehter it supports asynchronous operation
+  bool AsyncSupported = true;
+
----------------
Typo in comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81989