[CPU] Matmul emitters with TPP backend executor #28229
Conversation
chenhu-wang
force-pushed
the
chenhu/snippets_matmul_emitter_with_tpp_executor
branch
2 times, most recently
from
3091e05 to
51d7b93
Compare
|
@IvanNovoselov Could you please review? |
chenhu-wang
force-pushed
the
chenhu/snippets_matmul_emitter_with_tpp_executor
branch
from
51d7b93 to
592ef4c
Compare
| namespace ov { | ||
| namespace intel_cpu { | ||
|
|
||
| struct BrgemmTppKernelConfig : public BrgemmBaseKernelConfig { |
There was a problem hiding this comment.
I'm not sure that inheritance TPP config and executor from BrgemmBase is good idea.
As for config, BrgemmBaseKernelConfig has dnnl structures: dimensions, data type, isa (BTW tpp doesn't need in isa). When we use libxsmm, we should use libxsmm-specific structures. At the moment, StaticParams has dnnl_data_type fields and libxsmm_datatype fields. However, they mean the same.
As for BrgemmTppKernelExecutor, it's derived from BrgemmBaseKernelExecutor which works with dnnl structures too: create_brgemm_kernel(std::shared_ptr<dnnl::impl::cpu::x64::brgemm_kernel_t>& kernel ...). It means that BrgemmTppKernelExecutor has access to these methods but shouldn't.
In my opinion, TPP classes should not be derived from x64 onednn structures. If you want to reuse some interface, probably we should create the skeleton base class or utils static functions which will be available for all archs. I think that initialization of M,K,N, beta will be the same for all platforms. Please analyze if we can implement base class for all brgemm executors and configs for all platforms.
Moreover, if we want to implement executor with kernel impl from another library (kleidi, acl etc) - no one of them needs in dnnl types, they have own.
| } | ||
|
|
||
| bool BrgemmTppKernelConfig::StaticParams::operator==(const StaticParams& rhs) const { | ||
| return StaticBaseParams::operator==(rhs) && m_prefetching_flags == rhs.m_prefetching_flags; |
There was a problem hiding this comment.
Should we compare m_compile_flags and m_type_exec here too?
There was a problem hiding this comment.
The same question regarding BrgemmTppKernelConfig::StaticParams::to_string
| void BrgemmTppKernelExecutor::update_config(const ov::snippets::lowered::ExpressionPtr& expr, | ||
| const ov::snippets::lowered::LinearIRCPtr& linear_ir, | ||
| BrgemmTppKernelConfig& config) const { | ||
| BrgemmBaseKernelExecutor::update_config(expr, linear_ir, config); |
There was a problem hiding this comment.
At the beginning we update config. Then we update it one more time.. I think, this is sign when we should think about super-base class
| case ov::element::Type_t::i8 : return LIBXSMM_DATATYPE_I8; | ||
| case ov::element::Type_t::u8 : return LIBXSMM_DATATYPE_U8; | ||
| default: | ||
| OV_CPU_JIT_EMITTER_THROW("Attempt to convert unsupported ov data type"); |
There was a problem hiding this comment.
Minor: let's print the unsupported type in exception message:
| OV_CPU_JIT_EMITTER_THROW("Attempt to convert unsupported ov data type"); | |
| OV_CPU_JIT_EMITTER_THROW("Attempt to convert unsupported ov data type: ", element_type); |
|
|
||
| namespace ov { | ||
| namespace intel_cpu { | ||
| #define COMPILE_BRGEMM_TPP_KERNEL(...) \ |
There was a problem hiding this comment.
Can we reuse a similar macros from jit_tpp_emitter instead of creating a new one?
| } | ||
|
|
||
| bool BrgemmTppKernelConfig::StaticParams::operator==(const StaticParams& rhs) const { | ||
| return StaticBaseParams::operator==(rhs) && m_prefetching_flags == rhs.m_prefetching_flags; |
There was a problem hiding this comment.
The same question regarding BrgemmTppKernelConfig::StaticParams::to_string
Details:
Tickets: