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

[Guilherme Valarini via Phabricator via Openmp-commits]

gValarini added inline comments.


================
Comment at: openmp/libomptarget/include/omptarget.h:194
+  using PostProcFuncTy = std::function<int()>;
+  llvm::SmallVector<PostProcFuncTy> PostProcessingFunctions;
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132005