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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

I'll wait for the updated version to go through again. Below two clarification questions. I think I now am closer to understanding some of the stuff I was confused about. If we end up keeping this scheme we need to adjust some names. I am hoping we can simplify a few things though.



================
Comment at: openmp/libomptarget/src/private.h:227
+
+    delete AsyncInfo;
+    __kmpc_omp_set_target_async_handle(ExecThreadID, NULL);
----------------
gValarini wrote:
> jdoerfert wrote:
> > Should this be guarded by IsNew?
> Nope, `IsNew` indicates that a new task-attached `AsyncInfo` has just been allocated, but not that we should deallocate it or not. The variable is primarily used to indicate that we must dispatch new operations to the new handle. Maybe I should rename it to just `ShouldDispatch`. Deallocation is always done when `AsyncInfo->isDone()` returns `true`, which is previously checked.
I'm worried here that we might not delete the AsyncInfo or delete it multiple times. Are you saying there is exactly one TaskAsyncInfoTy that will own the AsyncInfor object at any given time? If not, how do we avoid double free?  


================
Comment at: openmp/libomptarget/src/private.h:240
+  /// synchronization is needed.
+  bool shouldDispatch() { return !IsNew; }
+};
----------------
gValarini wrote:
> jdoerfert wrote:
> > I don't understand what we want/need this dispatch idea for. It seems to skip operations but I don't understand how we would not forget about them and go back. 
> Here is the main idea: 
> 
> # The first time a target nowait (target inside an OpenMP task) is executed, a new `AsyncInfo` handle is created and stored in the OpenMP taskdata structured of said task. Since this is the first time we are executing it (which is detected by the `IsNew` variable), we dispatch the device side operations and populate the post-processing function array by calling the proper `omptarget.cpp` functions (e.g., `targetDataBegin`, `targetDataEnd`, ...).
> # Afterward, if the device operations are still pending, the OpenMP task is re-enqueued for execution.
> # Later, when the task is re-executed, the same outline function called at step 1 will be called again. Here we can recover the `AsyncInfo` handle from the OpenMP taskdata and just synchronize it. Since this time the handle is not new, we know the operations were already dispatched previously and we should not dispatch them again.
> 
> I have a [[ https://docs.google.com/presentation/d/1yFaIanlR4JccYFhrKe_30k17LgjvyvC6DkOgc1Hum5c/edit?usp=sharing | presentation]] that explains it on slides 19-24, but I believe I am failing to describe that in the code. I'll try to come up with some better documentation for this dispatch/synchronize idea.
Ok, that makes more sense. Now to help (even) me understand this, why do we need to call the functions from step 1 in step 3? We seem to use the "Dispatch" argument to skip most of what they do (the target data part of a targetDataXXX) anyway, no?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132005