Page MenuHomePhabricator

[OpenMP] Fixed the issue that target memory deallocation might be called when they're being used
ClosedPublic

Authored by tianshilei1992 on Jul 30 2020, 6:00 PM.

Details

Summary

This patch fixed the issue that target memory might be deallocated when
they're still being used or before they're used.

Diff Detail

Event Timeline

tianshilei1992 created this revision.Jul 30 2020, 6:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2020, 6:00 PM
tianshilei1992 requested review of this revision.Jul 30 2020, 6:00 PM

Thanks for fixing the bug. It should be good for the moment.
When I think about the existence of recursive mapper, we may still have more sync than needed. I think recursion the whole targetDataBegin/targetDataEnd is convenient but sub-optimal choice.
Recursion should only be done on the map/mapper analysis. Just leave my thoughts here. It needs a discussion beyond this patch.

openmp/libomptarget/src/omptarget.cpp
606

If targetDataEnd is not called from target.
AsyncInfo can be nullptr.
AsyncInfo->Queue also be nullptr in "target exit data nowait" if there is no transfer.

At the end of target(), the synchronize needs to check AsyncInfo->Queue.

tianshilei1992 added inline comments.Jul 30 2020, 7:07 PM
openmp/libomptarget/src/omptarget.cpp
606

There is no need to synchronize at the end of target. I forgot to remove it.
For other cases, let me think about them.

Thanks for fixing the bug. It should be good for the moment.
When I think about the existence of recursive mapper, we may still have more sync than needed. I think recursion the whole targetDataBegin/targetDataEnd is convenient but sub-optimal choice.
Recursion should only be done on the map/mapper analysis. Just leave my thoughts here. It needs a discussion beyond this patch.

Recursive mapper does introduce a lot of synchronization overhead as it passes nullptr to those functions, therefore in the plugin, it will first fetch a new stream, enqueue the operation, and synchronize. The whole mapper is using "synchronous" operations.

Removed the tailing synchronization because it is no use any more

Fixed issues in comments

tianshilei1992 marked an inline comment as done.Jul 30 2020, 7:39 PM
tianshilei1992 added inline comments.
openmp/libomptarget/src/omptarget.cpp
606

I added some check here. Nice catch.

[Drive By]

openmp/libomptarget/src/omptarget.cpp
434

Please use proper names even for argument. Also add doxygen comments to all members.

Updated the comments and arguments

"clause" -> "modifier"

tianshilei1992 marked an inline comment as done.Jul 31 2020, 11:50 AM
ye-luo accepted this revision.Jul 31 2020, 2:06 PM

LGTM

This revision is now accepted and ready to land.Jul 31 2020, 2:06 PM