[FORK][FIX][x64] Refactor avx2 binary PReLU and fix reg conflicts - #242
Conversation
|
|
||
| if (one_of(vmm_aux0, dst, lhs, rhs)) { | ||
| //let's find a vacant XMM register | ||
| int occupied_idices[] = {dst.getIdx(), lhs.getIdx(), rhs.isMEM() ? -1 : rhs.getIdx()}; |
There was a problem hiding this comment.
rhs should not be memory in my understanding for this template function.
std::enable_if<!(std::is_same<T, Xbyak::Zmm>::value
|| std::is_same<T, Xbyak::Address
There was a problem hiding this comment.
Yes, you are right. Thank you
| if (indx == 0) { | ||
| auto vmm_zero = Vmm(indx); | ||
| host_->vmovups(vmm_aux0, vmm_zero); | ||
| std::swap(vmm_aux0, vmm_zero); |
There was a problem hiding this comment.
Seems have some problem here.Assuming:
vmm_aux =vmm(4) , vmm_zero =vmm(0), lhs = vmm(0) before swapping. After swap, vmm_aux=vmm(0), vmm_zero=vmm(4), but lhs still vmm(0). Seem lhs should also be changed to vmm(4). right?
There was a problem hiding this comment.
Good catch. Will fix it
2da34c1 to
2d73bac
Compare
| } else { | ||
| using dnnl::impl::utils::one_of; | ||
| // in sse4 vmm_aux0 as mask it's index must be 0 | ||
| Vmm vmm_aux0 = Vmm(rhs_arg_static_params_.rhs_prelu_helper_vmm_idx); |
There was a problem hiding this comment.
The SSE implement assume that rhs_arg_static_params_.rhs_prelu_helper_vmm_idx = 0 and seems current binary_post_ops would set rhs_prelu_helper_vmm_idx = 0. However, Can we directly use vmm_aux0 = Vmm(0) and not use this rhs_prelu_helper_vmm_idx? Or add some check rhs_prelu_helper_vmm_idx must be 0, assert would not be compiled into release version. Maybe directly use vmm_aux0 = Vmm(0) ? It would be easier to understand this piece of code.
There was a problem hiding this comment.
In theory this rhs_arg_static_params_.rhs_prelu_helper_vmm_idx should be a hint that helps to avoid the search for the vacant register. Also since this is the SSE path, which is mostly supported as a legacy, I wouldn't put more effort on ironing out this part.
luweizhou2016
left a comment
luweizhou2016
left a comment
There was a problem hiding this comment.
Except the minor change on the comment. The code is okay for me.
2d73bac to
cdbfd4c
Compare
### Details: Refactor binary PReLU avx2 post op in order to reduce the amount of instructions for avx2 and fix the conflicting register indices issue for both avx2 and sse41. This is more advanced version of the previous fix ported only to the LTS branch #23910. Also dedicated SL tests were introduced. OneDNN fork PR openvinotoolkit/oneDNN#242 ### Tickets: - CVS-137492
2 participants
Description
OpenVINO PR openvinotoolkit/openvino#24102