[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 only have one remaining question. @tianshilei1992 might have more though.



================
Comment at: openmp/libomptarget/src/interface.cpp:331
+      Int64Envar("OMPTARGET_QUERY_COUNT_THRESHOLD", 5),
+      Envar<float>("OMPTARGET_QUERY_COUNT_BACKOFF_FACTOR", 0.5f));
+
----------------
Why is this thread_local? Should it be tied to the async info object with non-blocking tasks?


================
Comment at: openmp/libomptarget/src/omptarget.cpp:73
+  SmallVector<PostProcFuncTy> Functions(
+      std::move(PostProcessingFunctions));
+
----------------
gValarini wrote:
> jdoerfert wrote:
> > 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?
> I was thinking about a future use case: if a post-processing function generates more asynchronous device-side operations. In this case, it may want to add a new post-processing function into the vector, but we cannot do that while iterating over it. The [[ https://en.cppreference.com/w/cpp/container/vector/push_back | spec ]] says that, if a push/emplace back resizes the vector, any previous iterator is invalid, which would make the loop invalid. I think that is the case for `SmallVector`s as well.
> 
> Right now, the three post-processing functions do not do that. They all do synchronous device operations. But if in the future, someone adds a post-processing function that does that, it cannot be blindly done without taking care of this situation.
> 
> Do you think we could leave this "unimplemented" right now? If so, I can do the iteration-then-clear approach.
> 
> Just a "side question": which standard does `SmallVector` follows? I am asking that because the [[ https://en.cppreference.com/w/cpp/container/vector/vector | STL ]] says that a vector is guaranteed to be empty after a move. If that is the case for `SmallVectors`, thus `PostProcessingFunctions` would be in a valid state, no?
If you are worried about inserting, use
```for (int i = 0; i < C.size(); ++i)```

Anyhow, let's keep it this way for now but add an explicit clear at the end. Just to be sure (and explicit).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132005