This is an archive of the discontinued LLVM Phabricator instance.

[LIBOMPTARGET]Fix order of mapper data for targetDataEnd function.
ClosedPublic

Authored by ABataev on Aug 4 2020, 9:06 AM.

Details

Summary

targetDataMapper function fills arrays with the mapping data in the
direct order. When this function is called by targetDataBegin or
tgt_target_update functions, it works as expected. But targetDataEnd
function processes mapped data in reverse order. In this case, the base
pointer might be deleted before the associated data is deleted. Need to
reverse data, mapped by mapper, too, since it always adds data that must
be deleted at the end of the buffer.
Fixes the test declare_mapper_target_update.cpp.
Also, reduces the memry fragmentation by preallocation the memory
buffers.

Diff Detail

Event Timeline

ABataev created this revision.Aug 4 2020, 9:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2020, 9:06 AM
ABataev requested review of this revision.Aug 4 2020, 9:06 AM

So does the mapper function emit entries in reverse order upon exiting a target/target data region?

So does the mapper function emit entries in reverse order upon exiting a target/target data region?

No, it emits the data in the direct order. Currently, it emits in the following order: allocates, to/from/tofrom, deletes.
But targetDataEnd processes all records in reverse order and, thus, it results in the order: deletes, to/from/tofrom, allocs. So, at first, it deletes the data, and only after that, it tries to perform everything else.

After some thinking, seems to me the problem is much deeper. According to the standard, "For a given construct, the effect of a map clause with the to, from, or tofrom map-type is ordered before the effect of a map clause with the alloc, release, or delete map-type." So, looks to me, the compiler should reorder map info in the order to/from/tofrom, allocs/releases/deletes, and targetDataEnd function should process the mapping info in the direct order, just like tagetDataBegin and tgt_targe_update.

grokos accepted this revision.Aug 4 2020, 3:42 PM

OK, now it makes sense. LGTM

This revision is now accepted and ready to land.Aug 4 2020, 3:42 PM