Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gpu: generic: deconvolution: move implementation from ocl folder #2074

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

t4c1
Copy link
Contributor

@t4c1 t4c1 commented Sep 3, 2024

Move the fwd and bwd data implementations of deconv (convolution deconvolution) from opencl folder to generic one, as they are generic so they work with any convolution implementation. Also enables them for generic case.

This redoes the changes from #2020 that were reverted in 848b0f7 in a way that should be more acceptable.

@t4c1 t4c1 requested review from a team as code owners September 3, 2024 11:59
@github-actions github-actions bot added platform:gpu-intel Codeowner: @oneapi-src/onednn-gpu-intel platform:gpu-generic Codeowner: @oneapi-src/onednn-gpu-generic labels Sep 3, 2024
Comment on lines -54 to +61
GPU_INSTANCE_INTEL(intel::ocl::convolution_deconvolution_bwd_data_t)
GPU_INSTANCE_INTEL(intel::ocl::convolution_deconvolution_bwd_weights_t)
GPU_INSTANCE_NVIDIA(nvidia::cudnn_deconvolution_bwd_data_t)
GPU_INSTANCE_NVIDIA(nvidia::cudnn_deconvolution_bwd_weights_t)
GPU_INSTANCE_AMD(amd::miopen_deconvolution_bwd_data_t)
GPU_INSTANCE_AMD(amd::miopen_deconvolution_bwd_weights_t)
GPU_INSTANCE_GENERIC(generic::convolution_deconvolution_bwd_data_t)
Copy link
Contributor

@nwnk nwnk Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this have a performance impact for Intel? If there's a case where both would match, it would now match weights before data.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With these specific implementations, there should be no performance impact. They're mutually exclusive, looking for dnnl_backward_weights and dnnl_backward_data propagation kinds, respectively.

@mgouicem
Copy link
Contributor

mgouicem commented Sep 6, 2024

(nit) could you please use gpu: generic in the commit message ?

@mgouicem
Copy link
Contributor

mgouicem commented Sep 6, 2024

make test
disable device_cpu
enable device_gpu
enable thr_sycl
enable thr_ocl
enable thr_cuda
enable arch_rtx

@t4c1
Copy link
Contributor Author

t4c1 commented Sep 6, 2024

(nit) could you please use gpu: generic in the commit message ?

Done

@t4c1 t4c1 changed the title generic: deconvolution: move implementation from ocl folder gpu: generic: deconvolution: move implementation from ocl folder Sep 6, 2024
@@ -34,6 +34,9 @@
#include "gpu/generic/sycl/ref_deconvolution.hpp"
#endif

#include "gpu/generic/convolution_deconvolution.hpp"
#include "gpu/intel/ocl/convolution_deconvolution.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gpu/intel directory is not included in the build if gpu vendor is not intel. The header should always be guarded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this include, as it was a duplicate of the guarded one.

Comment on lines 25 to 27
#include "gpu/intel/compute/utils.hpp"
#include "gpu/intel/gpu_primitive.hpp"
#include "gpu/intel/primitive_conf.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generic implementations cannot include anything from vendor specific locations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@vpirogov vpirogov added this to the v3.7 milestone Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform:gpu-generic Codeowner: @oneapi-src/onednn-gpu-generic platform:gpu-intel Codeowner: @oneapi-src/onednn-gpu-intel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants