[Openmp-commits] [PATCH] D84487: [OpenMP] Add more pass-through functions in DeviceTy

[Ye Luo via Phabricator via Openmp-commits]

ye-luo added inline comments.


================
Comment at: openmp/libomptarget/src/device.h:198
+  /// succeeds/fails.
+  void *data_alloc(int64_t Size);
+  /// Deallocates memory which \p TgtPtrBegin points at and returns
----------------
tianshilei1992 wrote:
> ye-luo wrote:
> > tianshilei1992 wrote:
> > > ye-luo wrote:
> > > > tianshilei1992 wrote:
> > > > > ye-luo wrote:
> > > > > > tianshilei1992 wrote:
> > > > > > > Although `HstPtr` is not used here, it is still part of the plugin interface. In order to be compatible with the plugin interface, I think either `HstPtr` is kept here, or remove the `HstPtr` from the plugin interface.
> > > > > > My intention is to remove it fully.  Due to compatibility consideration, it has not been removed in the plugin interface. I insist in not exposing HstPtr here.
> > > > > I think it doesn't make any sense. The so-called compatibility is, we may use it somewhere or sometime, especially in the future. If the plugin interface leaves it, considering maybe in the future we may use it to assist the allocation of device memory, or something else, then there is no reason to remove it from `data_alloc` because `data_alloc` is also an interface.
> > > > Here I'm not intended to change __tgt_rtl_data_alloc in omptargetplugin.h. Any changes there requires a different discussion.
> > > > So far the added DeviceTy::data_alloc/data_delete are intended to allocate/deallocate device memory only so HstPtr is not part of this logic.
> > > I know. But `data_alloc` calls `__tgt_rtl_data_alloc` and it does have an argument for host pointer, right? Your intention is to take `data_alloc` as the only user of `__tgt_rtl_data_alloc`, and hope others to use `data_alloc` to allocate device memory in the future instead of using the function pointer directly. If `__tgt_rtl_data_alloc` has an argument for host pointer, why does `data_alloc` not have the argument?
> > Maybe my initial thoughts were not accurate.
> > `DeviceTy::data_alloc` is not a direct replacement of `__tgt_rtl_data_alloc`. It is intended for replacing all the calls `__tgt_rtl_data_alloc` currently used by libomptarget.so.
> > 
> > As long as `__tgt_rtl_data_alloc` interface stays as it is now. `DeviceTy::data_alloc` cannot fully cover it. Any codes which are not visible to us (downstream compilers) can still used them directly just like any other `__tgt_rtl` functions.
> > `DeviceTy::data_alloc` is intended to allocate device memory only and thus HstPtr is not part of the logic.
> > 
> > So far all the calls to `__tgt_rtl_data_alloc` in libomptarget.so are only intended for allocating device memory and HstBegin mapping to TgtBegin is done outside `__tgt_rtl_data_alloc call`.
> > For this reason, they can be all replaced by DeviceTy::alloc.
> > 
> > My intention of only using `DeviceTy::data_alloc` is not because of its covering all the `__tgt_rtl_data_alloc` feature but the real use we have seen so far only needs `DeviceTy::data_alloc`.
> Given that case, if you really don't want to pass a `nullptr` to it when using `data_alloc`, it would be better to define `data_alloc` as any of following:
> 1. `void *data_alloc(int64_t Size, void *HstPtr = nullptr)`;
> 2. Add a `void *data_alloc(int64_t Size, void *HstPtr)` and let `void *data_alloc(int64_t Size)` call `data_alloc(Size, nullptr)`.
> But I think the first one is less confusing since we only have one plugin interface and it does not have such distinction.
> What's more, `RTL` is a member variable of `DeviceTy`. we almost have all member functions in `DeviceTy` corresponding to those in `RTL`. We should keep "users" away from `RTL`, especially when they have to deal with `DeviceID` and `RTLDeviceID`. Otherwise it is confusing. When should I use member function of `DeviceTy`, and when should I use `RTL` directly and pass the right ID to it?
I will adopt 1.
A lot data member of DeviceTy should be private including RTL, DeviceID, RTLDeviceID. I was actually burnt by DeviceID and motivated this patch.
Is DeviceID actually get used?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84487/new/

https://reviews.llvm.org/D84487