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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a subscriber: kevinsala.
jdoerfert added a comment.

Generally fine from my end. @tianshilei1992 wdyt?

@kevinsala, FYI, there will be a new plugin API we need to port over to the new plugins.



================
Comment at: openmp/libomptarget/src/omptarget.cpp:73
+  SmallVector<PostProcFuncTy> Functions(
+      std::move(PostProcessingFunctions));
+
----------------
gValarini wrote:
> jdoerfert wrote:
> > This is not a good idea, I think. The state of PostProcessingFunctions is undefined afterwards, even if this works in practice. Simply iterate PostProcessingFunctions and then clear it.
> Uhm, really? When moving a `SmallVector` like this, wouldn't `PostProcessingFunctions` be emptied and all the values moved to `Functions`?
The standard says PostPRocessingFunctions is in an unspecified state after the move. If we look at SmallVector we know the state but I don't understand why we need to rely on implementation defined behavior here at all. We don't save any lines and also no work by just iterating PostProcessingFunctions and then clearing it, no?


================
Comment at: openmp/libomptarget/src/private.h:230
+    // never be any valid handle store inside the task at this point.
+    assert((*TaskAsyncInfoPtr) == nullptr);
+
----------------
All asserts should have messages.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132005