This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Use IsHostPtr where needed for targetDataEnd
ClosedPublic

Authored by jdenny on Aug 11 2021, 1:01 PM.

Details

Summary

As discussed in D105990, without this patch, targetDataEnd
determines whether to transfer data or delete a device mapping (as
opposed to assuming it's in shared memory) using two different
conditions, each of which is broken for some cases:

  1. !(UNIFIED_SHARED_MEMORY && TgtPtrBegin == HstPtrBegin): The broken case is rare: the device and host might happen to use the same address for their mapped allocations. I don't know how to write a test that's likely to reveal this case, but this patch does fix it, as discussed below.
  2. !UNIFIED_SHARED_MEMORY || HasCloseModifier: There are at least two broken cases:
    1. The close modifier might have been specified on an omp target enter data but not the corresponding omp target exit data, which thus might falsely assume a mapping is in shared memory. The test unified_shared_memory/close_enter_exit.c already has a missing deletion as a result, and this patch adds a check for that. This patch also adds the new test close_member.c to reveal a missing transfer and deletion.
    2. Use of discrete memory might have been forced by omp_target_associate_ptr, as in the test unified_shared_memory/api.c. In the current targetDataEnd implementation, this condition turns out not be used for this case: because the reference count is infinite, a transfer is possible only with an always modifier, and this condition is never used in that case. To ensure it's never used for that case in the future, this patch adds the test unified_shared_memory/associate_ptr.c.

Fortunately, DeviceTy::getTgtPtrBegin already has a solution: it
reports whether the allocation was found in shared memory via the
variable IsHostPtr.

After this patch, HasCloseModifier is no longer used in
targetDataEnd, and I wonder if the close modifier is ever useful
on an omp target data end.

Diff Detail

Event Timeline

jdenny created this revision.Aug 11 2021, 1:01 PM
jdenny requested review of this revision.Aug 11 2021, 1:01 PM
grokos accepted this revision.Aug 27 2021, 3:12 PM
grokos added inline comments.
openmp/libomptarget/src/device.cpp
390

Just to be on the safe side for future patches, can you add a comment saying that DeviceTy::deallocTgtPtr no longer checks whether we use USM and it assumes that the caller has checked beforehand that HstPtrBegin has (or should have) corresponding storage on the device instead of shared memory?

This revision is now accepted and ready to land.Aug 27 2021, 3:12 PM
jdenny added a comment.Sep 1 2021, 8:56 AM

@grokos: Thanks for the reviews. Should I go ahead and land this patch series (D107925, D107926, D107927, D107928) after applying your suggestions, or do you think it would be better to wait for the discussion in D108569 to settle?

grokos added a comment.Sep 1 2021, 9:09 AM

I think it's better to move on with these patches and then revisit D108569, which needs extensive changes anyway. It will be easier to work on that other patch if the fixes from this series have landed instead of trying to think about all corner cases all over again.

jdenny added a comment.Sep 1 2021, 9:11 AM

Sounds good. I'll work on this today.

This revision was automatically updated to reflect the committed changes.
jdenny marked an inline comment as done.
jdenny added inline comments.Sep 1 2021, 2:42 PM
openmp/libomptarget/src/device.cpp
390

I added the comment to device.h. I'm happy to revise in a fixup patch if you want something different.