Page MenuHomePhabricator

[OpenMP] Add more pass-through functions in DeviceTy
ClosedPublic

Authored by ye-luo on Jul 23 2020, 11:02 PM.

Details

Summary
  1. Add DeviceTy::data_alloc, DeviceTy::data_delete, DeviceTy::data_alloc, DeviceTy::synchronize pass-through functions. Avoid directly accessing Device.RTL
  2. Fix the type of the first argument of synchronize_ty in rth.h, device id is int32_t which is consistent with other functions.

Diff Detail

Event Timeline

ye-luo created this revision.Jul 23 2020, 11:02 PM

Thx for the cleanup, minor comments below.

openmp/libomptarget/src/device.cpp
357

I guess the HostPtr is never used by any RTL?

362

No function comments here then.

439

same as above.

openmp/libomptarget/src/device.h
197

While we are at it, comments for both, full sentences, explain the result of delete.

229

Same as above.

ye-luo marked an inline comment as done.Jul 24 2020, 6:57 AM
ye-luo added inline comments.
openmp/libomptarget/src/device.cpp
357

It seems never used.

362

Where you asking for comments of "DeviceTy::data_delete" or "return RTL->data_delete(RTLDeviceID, TgtPtrBegin);"?
For the former, if we have it in the header, do we still need to repeat the same comment at function definition?

ye-luo added inline comments.Jul 24 2020, 7:28 AM
openmp/libomptarget/src/device.cpp
362

https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
Don’t duplicate the documentation comment in the header file and in the implementation file.
I'm not sure I understand what you were asking for here.

ye-luo updated this revision to Diff 280468.Jul 24 2020, 8:18 AM
ye-luo marked 6 inline comments as done.
tianshilei1992 added inline comments.Jul 24 2020, 8:36 AM
openmp/libomptarget/src/device.h
198

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.

ye-luo added inline comments.Jul 24 2020, 8:51 AM
openmp/libomptarget/src/device.h
198

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.

tianshilei1992 added inline comments.Jul 24 2020, 9:01 AM
openmp/libomptarget/src/device.h
198

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.

ye-luo added inline comments.Jul 24 2020, 9:45 AM
openmp/libomptarget/src/device.h
198

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.

tianshilei1992 added inline comments.Jul 24 2020, 9:51 AM
openmp/libomptarget/src/device.h
198

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?

ye-luo added inline comments.Jul 24 2020, 10:32 AM
openmp/libomptarget/src/device.h
198

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.

openmp/libomptarget/src/device.h
198

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?

ye-luo added inline comments.Jul 24 2020, 12:34 PM
openmp/libomptarget/src/device.h
198

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?

tianshilei1992 added inline comments.Jul 24 2020, 1:10 PM
openmp/libomptarget/src/device.h
198

Yes, it is, but just in one case for now. :-)

ye-luo updated this revision to Diff 280582.Jul 24 2020, 2:03 PM
ye-luo marked an inline comment as done.Jul 24 2020, 2:06 PM
ye-luo added inline comments.
openmp/libomptarget/src/device.h
198

Done with adopting 1.

tianshilei1992 added inline comments.Jul 24 2020, 2:53 PM
openmp/libomptarget/src/omptarget.cpp
848

Please also add HstPtrBegin here.

ye-luo updated this revision to Diff 280608.Jul 24 2020, 3:36 PM
ye-luo marked an inline comment as done.
ye-luo added inline comments.
openmp/libomptarget/src/omptarget.cpp
848

done

This revision is now accepted and ready to land.Jul 24 2020, 7:58 PM
This revision was automatically updated to reflect the committed changes.