oneDNN
oneDNN copied to clipboard
openvinotoolkit
[FORK][x64] Remove custom binary PReLU post op
The gemm conv kernel diverges most with the upstream version, because fork enabled jit_pp kernel for jit gemm kernel but upstream still use ref_pp kernel.
https://github.com/openvinotoolkit/oneDNN/blob/26633ae49edd4353a29b7170d9fcef6b2d79f4b3/src/cpu/gemm_convolution.hpp#L77
shows that gemm_conv should not support prelu after this change. I just checked that we have test cases of checking prelu fusing on jit gemm kernel, but I didn't see these test cases are disabled on OV pr. can you pls help to double check? I think we need to add the prelu into the list. Some older platforms maybe would fallback on gemm conv on some cases.
Thank you for bringing attention to this. Need to check. Do I correctly understand that jit_pp kernel use the oneDNN post ops as is, and all we need is to simply add prelu to this list and the oneDNN prelu post op will work as it is within this jit_pp kernel?
The gemm conv kernel diverges most with the upstream version, because fork enabled jit_pp kernel for jit gemm kernel but upstream still use ref_pp kernel. https://github.com/openvinotoolkit/oneDNN/blob/26633ae49edd4353a29b7170d9fcef6b2d79f4b3/src/cpu/gemm_convolution.hpp#L77
shows that gemm_conv should not support prelu after this change. I just checked that we have test cases of checking prelu fusing on jit gemm kernel, but I didn't see these test cases are disabled on OV pr. can you pls help to double check? I think we need to add the prelu into the list. Some older platforms maybe would fallback on gemm conv on some cases.
Thank you for bringing attention to this. Need to check. Do I correctly understand that jit_pp kernel use the oneDNN post ops as is, and all we need is to simply add prelu to this list and the oneDNN prelu post op will work as it is within this jit_pp kernel?
Not sure but it should be. because finally jit pp kernel also use jit_uni_binary_injector to do the post-ops, maybe needs to change some passing argument to binary injector.
The gemm conv kernel diverges most with the upstream version, because fork enabled jit_pp kernel for jit gemm kernel but upstream still use ref_pp kernel. https://github.com/openvinotoolkit/oneDNN/blob/26633ae49edd4353a29b7170d9fcef6b2d79f4b3/src/cpu/gemm_convolution.hpp#L77
shows that gemm_conv should not support prelu after this change. I just checked that we have test cases of checking prelu fusing on jit gemm kernel, but I didn't see these test cases are disabled on OV pr. can you pls help to double check? I think we need to add the prelu into the list. Some older platforms maybe would fallback on gemm conv on some cases.
Thank you for bringing attention to this. Need to check. Do I correctly understand that jit_pp kernel use the oneDNN post ops as is, and all we need is to simply add prelu to this list and the oneDNN prelu post op will work as it is within this jit_pp kernel?
Not sure but it should be. because finally jit pp kernel also use jit_uni_binary_injector to do the post-ops, maybe needs to change some passing argument to binary injector.
- There were no PReLU per channel SL tests, so all the existing PReLU post ops tests of conv layers were per tensor, and the prelu post op was eventually mapped to the eltwise post op.
- I extended the conv SL tests with PReLU per channel, but normally those post ops are performed via
dnnl_depthwise_prelu, i.e. the legacy code path. So I brought back the depthwise post op check to the reference post ops implementation, to support prelu being performed via the legacy depthwise post op for reference convolution implementations. - I extended jit_pp_kernel with all the necessary checks for the original oneDNN prelu post op. I tested this code path with SL tests with PReLU per channel post ops but with a custom build, where legacy post ops are disabled. The jit_pp_kernel works with the original oneDNN prelu post op. At least I didn't observe any functional issues.
Hope this answers your initial question