Page MenuHomePhabricator

[OpenMP][FIX] Avoid use of stack allocations in asynchronous calls
ClosedPublic

Authored by jdoerfert on Feb 14 2021, 10:32 AM.

Details

Summary

As reported by Guilherme Valarini [0], we used to pass stack allocations
to calls that can nowadays be asynchronous. This is arguably a problem
and it will inevitably result in UB. To remedy the situation we
allocate the locations as part of the AsyncInfoTy object. The lifetime
of that object matches what we need for now. If the synchronization is
not tied to the AsyncInfoTy object anymore we might need to have a
different buffer construct in global space.

This should be back-ported to LLVM 12 but needs slight modifications as
it is based on refactoring patches we do not need to backport.

[0] https://lists.llvm.org/pipermail/openmp-dev/2021-February/003867.html

Diff Detail

Event Timeline

jdoerfert created this revision.Feb 14 2021, 10:32 AM
jdoerfert requested review of this revision.Feb 14 2021, 10:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2021, 10:32 AM
Herald added a subscriber: sstefan1. · View Herald Transcript
lebedev.ri added inline comments.
openmp/libomptarget/src/omptarget.cpp
35

?

JonChesterfield accepted this revision.Feb 14 2021, 12:48 PM
JonChesterfield added a subscriber: JonChesterfield.

LG. Why deque over list?

This revision is now accepted and ready to land.Feb 14 2021, 12:48 PM
tianshilei1992 added inline comments.Feb 14 2021, 1:04 PM
openmp/libomptarget/include/omptarget.h
162

I'd go with a more generic method:

std::vector<std::unique_ptr<char[]>> BufferLocations;
template <typename T>
T *getPtr() {
  BufferLocations.emplace_back(std::make_unique<char[]>(sizeof(T)));
  return reinterpret_cast<T *>(BufferLocations.back().get());
}
jdoerfert marked an inline comment as done.Feb 14 2021, 3:52 PM

LG. Why deque over list?

Some small memory overhead sounds better than linear allocation cost.

@tianshilei1992 Can you back port a variation of this to LLVM 12?

openmp/libomptarget/include/omptarget.h
162

That seems overkill as long as we only get void* out of this. My method avoids one level of indirection and the unique pointer has the same lifetime as the vector anyway.

jdoerfert updated this revision to Diff 323644.Feb 14 2021, 3:52 PM

Minor update

jdoerfert edited the summary of this revision. (Show Details)Feb 14 2021, 3:52 PM
tianshilei1992 added a comment.EditedFeb 14 2021, 7:35 PM

LG. Why deque over list?

Some small memory overhead sounds better than linear allocation cost.

@tianshilei1992 Can you back port a variation of this to LLVM 12?

I suggest to land all related patches because they're all dependent. Having a variant is like doing all these things again.

openmp/libomptarget/include/omptarget.h
162

I'm fine with your method. It's just too "tricky". Having a queue of 4B/8B objects is not so "natural".

This revision was landed with ongoing or failed builds.Feb 16 2021, 1:38 PM
This revision was automatically updated to reflect the committed changes.