[Openmp-commits] [PATCH] D132005: Add non-blocking support for target nowait regions

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added inline comments.


================
Comment at: openmp/libomptarget/include/omptarget.h:194
+  using PostProcFuncTy = std::function<int()>;
+  llvm::SmallVector<PostProcFuncTy> PostProcessingFunctions;
+
----------------
gValarini wrote:
> jdoerfert wrote:
> > Drive by: I don't believe we want such a generic interface. The postprocessing task should just be a fixed function, not multiple unknown at compile time.
> Just adding some context to why it was done this way:
> 
>   - Lambdas can easily store any local data needed by the post-processing procedures. This allows them to be generated locally by the normal code flow (which may improve code maintenance) and then stored in the lambdas implicit structure (solving any lifetime problems).
>   - Having a function vector allows us to easily compose post-processing procedures. This is the case for `target nowait` regions, that should run both the `targetDataEnd` and `processDataAfter` post-processing functions.
>   - If in the future we need to generate more asynchronous operations inside the post-processing functions, this can be easily done by pushing more lambdas to the vector.
> 
> With all that in mind, I know `std::function`s may lead to an additional heap allocation and one more level of indirection due to type-erasure, prohibiting some code opts. If that is not desirable despite the presented context, I can change how the post-processing procedures are stored/called, but I really would like to keep some of the points described above, especially the lifetime correctness of the post-processing data. Do you have something in mind that could help me with that? 
> 
> Maybe defining a different struct for each post-processing function and storing it in the `AsyncInfoTy` as a `variant` could be enough. What do you think?
I would have assumed if we have 2 potential callbacks, let's say static functions `F` and `G` we would have two members, both void*.

```
void *PayloadForF = nullptr;
void *PayloadForG = nullptr;
```

and if they are not null we call `F` and `G` respectively passing the payload.

I'm fine with keeping it for now this way, we can see if there is a need to change it.



================
Comment at: openmp/libomptarget/include/omptargetplugin.h:160
+// asynchronous operations, marking the internal queue as nullptr when
+// completed.
+int32_t __tgt_rtl_synchronize_async(int32_t ID, __tgt_async_info *AsyncInfo);
----------------
Describe the return value, it's not clear what would be returned if it does or doesn't synchronize.


================
Comment at: openmp/libomptarget/src/device.cpp:634-635
+    return RTL->synchronize_async(RTLDeviceID, AsyncInfo);
+  else
+    return synchronize(AsyncInfo);
+}
----------------



================
Comment at: openmp/libomptarget/src/interface.cpp:143
+    Rc = AsyncInfo->synchronize(SyncType);
+  handleTargetOutcome(Rc == OFFLOAD_SUCCESS, Loc);
+
----------------
I assume we can outline some of this as it is probably the same as in the sync version, right? Let's avoid duplication as much as possible. Same below.


================
Comment at: openmp/libomptarget/src/interface.cpp:282
+                          ArgTypes, ArgNames, ArgMappers, *AsyncInfo);
+  }
+  if (Rc == OFFLOAD_SUCCESS)
----------------
So, when do we call this but don't want to actually do `targetDataUpdate`? I am confused. Same above.


================
Comment at: openmp/libomptarget/src/interface.cpp:405
+  return OMP_TGT_SUCCESS;
 }
 
----------------
Clang format.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:698
 
+static int
+postProcessingTargetDataEnd(DeviceTy *Device,
----------------
Documentation, plz.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:1205
                           const map_var_info_t HstPtrName = nullptr)
-        : Index(Index), HstPtrBegin(reinterpret_cast<const char *>(HstPtr)),
+        : Index(Index), HstPtrBegin(reinterpret_cast<char *>(HstPtr)),
           HstPtrEnd(HstPtrBegin + Size), AlignedSize(Size + Size % Alignment),
----------------
Why are the const problematic?


================
Comment at: openmp/libomptarget/src/omptarget.cpp:1517
+        return Ret;
+      });
 
----------------
FWIW, `mutable` is really not my favorite way of handling things.


================
Comment at: openmp/libomptarget/src/private.h:214-217
+  if (AsyncInfo->isDone()) {
+    delete AsyncInfo;
+    __kmpc_omp_set_target_async_handle(GTID, NULL);
+  }
----------------
Here and elsewhere, we should prefer early exit and no else after return.


================
Comment at: openmp/libomptarget/src/private.h:224-228
+  if (__kmpc_omp_has_task_team(GTID)) {
+    return AsyncInfoTy::SyncType::NON_BLOCKING;
+  } else {
+    return AsyncInfoTy::SyncType::BLOCKING;
+  }
----------------



================
Comment at: openmp/runtime/src/kmp_tasking.cpp:1145
+      KMP_ATOMIC_DEC(&__kmp_unexecuted_hidden_helper_tasks);
+    }
   }
----------------
@tianshilei1992 you need to look at these changes.


================
Comment at: openmp/runtime/src/kmp_tasking.cpp:5187
+ at return Returns true if the current thread of the given thread has a task team
+allocated to it.
+
----------------
current task?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132005