x64: utils: jit_io_helper: fix xf16 store from Xmm (fixes MFDNN-12635) #2509
Conversation
github-actions
bot
added
platform:cpu-x64
|
make test |
| host_->vcvtneps2bf16(cvt_lower_vmm, vmm, Xbyak::VexEncoding); | ||
| host_->vcvtneps2bf16(cvt_lower_vmm, vmm, | ||
| mayiuse(avx512_core) ? Xbyak::EvexEncoding | ||
| : Xbyak::VexEncoding); |
There was a problem hiding this comment.
How come an instruction that supports only Evex encoding has an option for Vex encoding?
There was a problem hiding this comment.
Good question, but according to xbyak it has parameter for encoding:
https://github.com/oneapi-src/oneDNN/blob/main/third_party/xbyak/xbyak_mnemonic.h#L1260
And I see it's not the only place where Vex encoding is used, for example pooling:
https://github.com/oneapi-src/oneDNN/blob/main/src/cpu/x64/jit_uni_pool_kernel.cpp#L931
There was a problem hiding this comment.
I think we should limit that instruction to Evex only, regardless if xbyak provides an opportunity for encoding argument.
There was a problem hiding this comment.
I'll do separate PR for this, as it occurs in many files.
|
|
||
| # Test bf16 with aBcde4b format | ||
| --reset | ||
| --skip-impl=ref,simple # ! test jit version only |
There was a problem hiding this comment.
| --skip-impl=ref,simple # ! test jit version only | |
| --skip-impl=simple #skip non-jit version |
tczeszun
force-pushed
the
tczeszun/fix_io_helper_xf16_xmm
branch
from
c1c72ef to
3d10aca
Compare
|
make test |
This is a short fix for MFDNN-12635.
Function
jit_io_helper_t<Vmm>::store_bf16doesn't supportXmm, that's why it crashed when usingbf16with blocked format with channels blocked by 4.I changed a way it works to use
jit_io_helper_t<Vmm>::store_byte_by_bytefunction instead.Though, I have other idea that maybe using
store_bf16could work if we introduced anotherOpmaskto filter out the lower half ofXmm. It would require some changes tojit_io_helper_tclass, so I prefer to not submit it now as a bugfix, but after some time (as it requires some analysis), that's why I left aTODOcomment.Also I'm adding reorder regression input to benchdnn, as it was likely not covered on CPU side.