[SYCL][ESIMD][EMU] PI_API debug for esimd_emulator plug-in #5606
Conversation
dongkyunahn-intel
commented
Feb 17, 2022
dongkyunahn-intel
commented
- Enabling piEnqueueMemBufferMap/Unmap
- Serializing acces to Addr2CmBufferSVM
- Enabling piEnqueueMemBufferMap/Unmap - Serializing acces to Addr2CmBufferSVM
- CM-generated SurfaceIndex is not used in ESIMD_EMULATOR Plug-in
- Unused functions are removed - sycl_get_cm_buffer/image_params - Interface functions for getting surface info are renamed to not have 'cm' as they are used for surfaces generated by both CM and Host - As there is no legacy from previous productization, interface functions can be revised for now while keeping interface version as v1
- in order to prevent memory corruption error - fixes memory corruption error from '$TEST_SUITE/ESIMD/SYCL/api/simd_any_all.cpp'
- 'std::stack' for add/removeMapping from piEnqueueMemBufferMap/Unmap replacing 'std::unordered_map' - Common surface creation argument sanity check - Changes in atomic operations
|
/verify with intel/llvm-test-suite#937 |
|
/verify with intel/llvm-test-suite#937 |
| @@ -119,15 +119,12 @@ static sycl::detail::ESIMDEmuPluginOpaqueData *PiESimdDeviceAccess; | |||
| // Single-entry cache for piPlatformsGet call. | |||
| static pi_platform PiPlatformCache; | |||
| // TODO/FIXME : Memory leak. Handle with 'piTearDown'. | |||
| static sycl::detail::SpinLock *PiPlatformCacheMutex = | |||
| new sycl::detail::SpinLock; | |||
| static std::mutex *PiPlatformCacheLock = new std::mutex; | |||
There was a problem hiding this comment.
Mustn't it be initialized in init function? (and all other similar places)
There was a problem hiding this comment.
Will cover this with other similar variable initialization. (#5599)
| @@ -458,8 +456,7 @@ __esimd_scatter_scaled(__ESIMD_DNS::simd_mask_storage_t<N> pred, | |||
| uint32_t width; | |||
| std::mutex *mutexLock; | |||
|
|
|||
| I->sycl_get_cm_buffer_params_index_ptr(surf_ind, &writeBase, &width, | |||
| &mutexLock); | |||
| I->sycl_get_cm_buffer_params_ptr(surf_ind, &writeBase, &width, &mutexLock); | |||
There was a problem hiding this comment.
Why passing by pointer and not reference?
There was a problem hiding this comment.
Pointer is more clear & explicit on the caller side.
There was a problem hiding this comment.
Plus sycl_get_cm_buffer_params_ptr is a part of C interface, not C++.
| @@ -74,6 +74,8 @@ struct _pi_device : _pi_object { | |||
| : Platform{ArgPlt}, CmDevicePtr{ArgCmDev}, VersionStr{ArgVersionStr} {} | |||
|
|
|||
| pi_platform Platform; | |||
| // TODO: Check if serialization is required when ESIMD_EMULATOR | |||
| // plug-in calls CM runtime functions | |||
| cm_support::CmDevice *CmDevicePtr = nullptr; | |||
There was a problem hiding this comment.
can it be & instead of * ?
I mean, for C++ using raw pointers when references can be used (no pointer reassignment/arithmetics/etc required) is questionable
There was a problem hiding this comment.
This is how other plug-ins are creating and managing device info.
There was a problem hiding this comment.
How do you make a reference to nullptr?
There was a problem hiding this comment.
How do you make a reference to nullptr?
My question is: is nullptr needed in this case?
_pi_device is constructed with CmDevice and without it it makes no sense:
std::unique_ptr<_pi_device> Device(
new _pi_device(this, CmDevice, StrFormat.str()));
There was a problem hiding this comment.
Is there specific problem you are seeing with this code? Or is it just stylistic consideration? I don't think this should hold this PR then.
There was a problem hiding this comment.
I'll address this pointer/nullptr/reference problem in a separate PR if needed.
| std::unordered_map<void *, cm_support::CmBufferSVM *> Addr2CmBufferSVM; | ||
| // A lock guarding access to Addr2CmBufferSVM | ||
| std::mutex Addr2CmBufferSVMLock; |
There was a problem hiding this comment.
Interesting if synchronized map access could be encapsulated here, not scattered around plugin (?)
There was a problem hiding this comment.
Will discuss with similar access controls (#5599)
There was a problem hiding this comment.
Let's make it a separate PR in any case.
There was a problem hiding this comment.
I'll post another PR for discussing this.
| // Use std::stack for strict LIFO behavior checking? | ||
| std::unordered_map<void *, Mapping> Mappings; | ||
| // Supporing multi-threaded mapping/unmapping calls | ||
| std::mutex MappingsMutex; | ||
|
|
||
| virtual ~_pi_mem() = default; |
There was a problem hiding this comment.
The destructor is virtual, however base class (_pi_object) destructor is not.
There was a problem hiding this comment.
This is imported from level_zero. Will update _pi_object here.
There was a problem hiding this comment.
OK, just to make sure: if base will have virtual destructor it'd make sense to update all the descendant classes, not just this one (for now I see only this one having virtual destructor)
All in all it's either virtual or not if usage model allows, but it should be consistent.
There was a problem hiding this comment.
Couple of notes:
- Those added virtual destructors don't do anything and unlikely will, so this is just extra lines of code to maintain. I don't think they should've been added. If _pi_buffer needs to execute some additional code in destructor, that's when
virtual ~pi_buffer::pi_buffer()should be added, but not as=default - there is no need in default virtual destructor in _pi_mem as well
There was a problem hiding this comment.
Per Konst's comment, I'll remove all virtual destructors.
There was a problem hiding this comment.
As mentioned above it just needs to be consistent. Virtual or not - depends on usage model: virtual destructor is needed in base classes if you will delete the object using pointer to a base class and have code to execute in derived object destructor which also involves having any members with non-trivial destructors (which automatically makes the containing object's destructor non-trivial).
Me personally would have base class destructor virtual as I'm not sure that PI objects are guaranteed not to be deleted via pointers to _pi_object along with guarantee that class hierarchy will never contain non-trivial destructors.
|
Pass/Fails from CI tests Unexpected passes Unrelated failure Unrelated failure |
| // TODO/FIXME : Memory leak. Handle with 'piTearDown'. | ||
| static sycl::detail::SpinLock *PiESimdSurfaceMapLock = | ||
| new sycl::detail::SpinLock; | ||
| static std::mutex *PiESimdSurfaceMapLock = new std::mutex; |
There was a problem hiding this comment.
why the TODO was removed?
There was a problem hiding this comment.
Will recover.
There was a problem hiding this comment.
@dongkyunahn-intel , a question: why just not to fix those global initialization right away? What's the point not to do it properly by initializing and de-initializing in corresponding plugin API ?
I understand that it can be done in a separate commit, but why just not to fix it right away?
|
Given that
I'll merge once testing is finished. |
|
@dongkyunahn-intel, please create a follow-up PR to address @lsatanov's comments. We can discuss this on the meeting. |
