[Openmp-commits] [PATCH] D65001: [OpenMP][libomptarget] Add support for unified memory for regular maps
Jonas Hahnfeld via Phabricator via Openmp-commits
openmp-commits at lists.llvm.org
Tue Jul 23 06:38:21 PDT 2019
Hahnfeld requested changes to this revision.
Hahnfeld added a comment.
This revision now requires changes to proceed.
I'd still like to see tests as mentioned in my last comment.
Comment at: libomptarget/src/api.cpp:118-123
+ // Under unified memory the host pointer can be returned by the
+ // getTgtPtrBegin() function which means that there is no device
+ // corresponding point for ptr. This function should return false
+ // in that situation.
+ if (Device.RTLRequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY)
+ rc = (TgtPtr != ptr);
> Hahnfeld wrote:
> > The spec says `This routine returns non-zero if the specified pointer would be found present on device device_num by a map clause; otherwise, it returns zero.` Programs with `unified_shared_memory` could imply that all pointers are present on all devices, so I would have expected the routine to always return true. What's the reasoning for checking that the pointers differ?
> I'm checking if the pointer is on the actual device. If unified memory is used then the pointers will match and the device present test will return false.
> I have now refactored this check to make it more precise: if the host pointer is used then we have a flag for that.
Okay, I think I get what you're saying: You want to check that this particular pointer has been `map`ped to that specific `device_num`, right?
But with unified shared memory, this shouldn't matter, right? Because all pointers can be accessed from all devices, no?
Comment at: libomptarget/src/device.cpp:207-215
+ uintptr_t tp;
+ // If unified memory is active AND a use_device_ptr clause was used,
+ // we will force the allocation of a device variable. It means that the
+ // user really wants to have this variable replicate on the device.
+ if (RTLRequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY &&
+ tp = (uintptr_t)RTL->data_alloc(RTLDeviceID, Size, NULL);
> I don't understand this comment and behavior: `use_device_ptr` is a clause for `target data` and IIRC it means that the pointer is replaced by the device pointer in the data region. Why does this imply we have to allocate memory? I'd expect to just keep the host pointer because all memory of all devices is accessible from the host.
> Can you maybe add a test for the expected behavior?
I still don't understand this.
CHANGES SINCE LAST ACTION
More information about the Openmp-commits