[SYCL] Drop the old post-wait graph cleanup mechanism #7727
Conversation
sergey-semenov
changed the title
sergey-semenov
changed the title
|
Still need to add tests, but the runtime changes should be ready for review. |
| @@ -1036,22 +1033,17 @@ void Scheduler::GraphBuilder::decrementLeafCountersForRecord( | |||
| MemObjRecord *Record) { | |||
| for (Command *Cmd : Record->MReadLeaves) { | |||
| --(Cmd->MLeafCounter); | |||
| if (Cmd->MLeafCounter == 0 && Cmd->isSuccessfullyEnqueued() && | |||
| Cmd->supportsPostEnqueueCleanup()) | |||
| if (Cmd->readyForCleanup()) | |||
There was a problem hiding this comment.
Not being familiar with the codebase, so possibly nonsense...
Any chance that having cleanupCommandIfReady would be a better interface now after the refactoring has happened?
There was a problem hiding this comment.
I'd rather keep it as is. One of the places where readyForCleanup() check is currently made is in updateLeaves, which cannot clean up commands on the spot (since some of its potential callers don't hold the graph lock), so we would need to keep that check regardless. In the other three cases we could go with cleanupCommandIfReady(), but I don't think the current version is overly verbose to warrant introducing an additional function.
| return Buf_.get_access<sycl::access::mode::read_write>( | ||
| CGH, range<1>(BufferSize_), id<1>(OffsetSize)); |
There was a problem hiding this comment.
Can we use accessor's CTAD here?
There was a problem hiding this comment.
I'm having trouble using these in our library code, getting a CTAD compilation error with gcc (clang works just fine, we don't see any issues in SYCL applications after all). Not sure why, I'll look into it and address the stylistic accessor changes in this file separately, if you don't mind.
| auto FlushBufAcc = FlushBuf_.get_access<access::mode::discard_write, | ||
| access::target::host_buffer>( |
There was a problem hiding this comment.
| auto FlushBufAcc = FlushBuf_.get_access<access::mode::discard_write, | |
| access::target::host_buffer>( | |
| host_accessor FlushBufAcc{sycl::write, sycl::no_init}; |
| .StreamBuffersPool.find(this) | ||
| ->second->FlushBuf | ||
| FlushBuf_ |
There was a problem hiding this comment.
nit: host_accessor
| // and associates it with the provided Data. | ||
| template <class T> inline T createDummyHandleWithData(unsigned char *Data) { | ||
| DummyHandlePtrT DummyHandlePtr = new DummyHandleT(Data); | ||
| return reinterpret_cast<T>(DummyHandlePtr); |
There was a problem hiding this comment.
Why doesn't it violate ANSI-alias rules?
There was a problem hiding this comment.
This is a helper function for creating underlying data structure for native handles in the mock plugin (e.g. pi_mem). Like with proper plugins, these handles are never dereferenced directly and only used inside mock plugin's PI functions after reinterpret casting back to DummyHandlePtrT.
|
@romanovvlad Could you please take a look at the added tests as well? |
|
The test changes introduced shared libs compilation failure, I'll open a PR with the fix. |
|
Opened #7794 |
This patch updates the remaining cases that still used the old
post-wait graph cleanup:
of internal buffers allocated for them is now handled like any other
buffer with deferred release.
Since their resources are not limited to buffers, we now regularly check
the status of such kernels alongside regular graph cleanup, and release
the resources when we're able to.
sent to the new cleanup mechanism instead of relying on the old one.