[New Operator] Initial Implementation of AverageBlur - #168
[New Operator] Initial Implementation of AverageBlur#168daniellegillai wants to merge 23 commits into
Conversation
|
Current design: Use the 2D kernel when the max kernel size the operator is constructed with is 3x3. Otherwise use the linearly separated optimization. |
zacharyvincze
left a comment
zacharyvincze
left a comment
There was a problem hiding this comment.
A few issues come up since this is a stateful operator and has a handful of caveats compared to the usual stateless operators. For now, due to time constraints, it would be better to document the limitations in the operator's header itself and file an issue in the repo to track and come back to a solution to this later on.
| } | ||
| processAnchor(anchorX, anchorY, kernelWidth, kernelHeight); | ||
|
|
||
| std::lock_guard<std::mutex> lock(m_bufferMutex); |
There was a problem hiding this comment.
Potential race condition when calling the operator using the GPU then calling the same operator on the CPU afterwards. Because GPU ops are async, the operator() call can release the lock while the GPU is still doing work. If the operator is called on the CPU during previous GPU work, it will pick up the lock (and not synchronize to the hip event, since that is gated on it being a GPU call) and potentially modify the contents of m_hostKernelMem while the GPU is copying.
Should be able to fix this by having both versions synchronize to m_completionEvent rather than making that GPU only. The host-side lock is redundant in this case.
There was a problem hiding this comment.
An event synchronize ruins the asynchronous nature of this operator when pipelining, which has been discussed before. But we'll need to come up with a better solution when it comes to these stateful operators later on.
There was a problem hiding this comment.
Concluded that host side lock is still needed to synchronize the CPU work in a multithreaded scenario
There was a problem hiding this comment.
02008bb: Always synchronizes on the hip event, on both cpu and gpu device types. This way if the operator was previously called on GPU, the CPU call will wait for the GPU asynchronous work to complete before touching the CPU memory (that may still be being copied over to the GPU).
There was a problem hiding this comment.
6f4f20f: Documents the serialization of the operator in the header
| eDataType, std::array<std::function<void(hipStream_t, const Tensor&, const Tensor&, float*, float*, int, int, int, int, eBorderType, eDeviceType)>, 4>> | ||
| funcs = | ||
| { | ||
| {eDataType::DATA_TYPE_U8, {dispatch_filter_2d_dtype_separable<uchar1, float*>, 0, dispatch_filter_2d_dtype_separable<uchar3, float*>, dispatch_filter_2d_dtype_separable<uchar4, float*>}}, |
There was a problem hiding this comment.
The seperable filter2d kernel launch path requests a certain amount of shared memory that's based on the size of said kernel. Since there is no bounding of the kernel size, it's possible that it may request more LDS than is available on the card..
|
Another note. Since this follows the same concurrency logic as Gaussian (and has issues that went uncaught in my initial review of it), the same rules apply and we'll need to document the limitations/file an issue for that operator as well. |
|
Issue #182 made about the stateful / serialization constraint |
|
PR #183 applies the synchronization fix to the Gaussian operator too |
2 participants
Motivation
Initial implementation of the AverageBlur operator.
Technical Details
Reuses the same Filter2D kernels as Gaussian.
Currently using the separated horizontal and vertical kernels, but benchmarking shows that for 3x3 kernels the 2d version is faster, so may implement different paths.
Also follows the same concurrency control methods as Gaussian.
Test Plan
Tests added for C++ and Python implementations.
Test Result
C++ and Python tests all pass.
Submission Checklist