[CPU] Implementation of jit_kernel_base#12991

Closed

lohika-denis-kotov wants to merge 4 commits into

openvinotoolkit:masterfrom

lohika-denis-kotov:feature/jit_kernel_base_class

Contributor

lohika-denis-kotov commented Sep 9, 2022 •

edited

Loading

Description

Implementation of jit_kernel_base which makes implementation of all kernels more consistent and allows to share common functionality between kernels.

This PR depends on [CPU] StackAllocator for JIT kernels

Usage

struct jit_refine_anchors_conf {
    ...
};

struct jit_refine_anchors_call_args {
    ...
};

using jit_refine_anchors_kernel = jit_kernel_base<jit_refine_anchors_conf, jit_refine_anchors_call_args>;

template <x64::cpu_isa_t isa>
class jit_refine_anchors_kernel_fp32 : public jit_refine_anchors_kernel {
 public:
    DECLARE_CPU_JIT_AUX_FUNCTIONS(jit_refine_anchors_kernel_fp32)
    ...
    using Vmm = typename jit_kernel_traits<isa, ov::element::Type_t::f32>::Vmm;
    static constexpr unsigned VCMPPS_LE = jit_kernel_traits<isa, ov::element::Type_t::f32>::VCMPPS_LE;
    static constexpr unsigned VCMPPS_GT = jit_kernel_traits<isa, ov::element::Type_t::f32>::VCMPPS_GT;
    static constexpr unsigned SIMD_WIDTH = jit_kernel_traits<isa, ov::element::Type_t::f32>::SIMD_WIDTH;
    ...
    jit_refine_anchors_kernel_fp32(const jit_refine_anchors_conf &jqp)
        : jit_refine_anchors_kernel(isa, jqp) {}
    ...
    void generate() override;
    ...
};

lohika-denis-kotov requested review from a team as code owners

September 9, 2022 09:12

Contributor Author

lohika-denis-kotov commented Sep 9, 2022

@dmitry-gorokhov @ceciliapeng2011 Please, take a look at this review and assign reviwers

lohika-denis-kotov mentioned this pull request

[CPU] The registers pool implementation for JIT kernels. #12880

Merged

nshchego reviewed

View reviewed changes

src/plugins/intel_cpu/src/nodes/kernels/jit_kernel_base.hpp Outdated

nshchego reviewed

View reviewed changes

src/plugins/intel_cpu/src/nodes/kernels/jit_kernel_base.hpp Outdated

lohika-denis-kotov force-pushed the feature/jit_kernel_base_class branch from b87358f to 62ebd99 Compare

September 15, 2022 10:23

lohika-denis-kotov requested a review from nshchego

September 15, 2022 10:29

lohika-denis-kotov changed the title ~~Implementation of jit_kernel_base~~ [CPU] Implementation of jit_kernel_base

maxnick added the category: CPU label

maxnick self-assigned this

maxnick reviewed

View reviewed changes

src/plugins/intel_cpu/src/nodes/kernels/jit_kernel_base.hpp Outdated

lohika-denis-kotov force-pushed the feature/jit_kernel_base_class branch from 62ebd99 to be8b3f1 Compare

September 21, 2022 12:05

lohika-denis-kotov requested review from a team as code owners

September 21, 2022 12:05

lohika-denis-kotov mentioned this pull request

Add uni_gatherdps and uni_scatterdps openvinotoolkit/oneDNN#145

Closed

lohika-denis-kotov force-pushed the feature/jit_kernel_base_class branch from c79e5b3 to ca9f793 Compare

September 22, 2022 18:03

nshchego reviewed

View reviewed changes

src/plugins/intel_cpu/src/nodes/kernels/jit_kernel_base.hpp Outdated

+                  inline void uni_vgatherdps(const Xbyak::Xmm &xmm_val,
+                                             const Xbyak::Reg64 &reg_addr,
+                                             const Xbyak::Xmm &xmm_index,
+                                             const int &scale,

Contributor

nshchego Oct 3, 2022

Better to set default values for scale and disp and do nothing for that values. Also need to check them for acceptable values.

lohika-vmostovy Nov 3, 2022

uni_vgatherdps and uni_vscatterdps were excluded from this PR as you requested.

nshchego reviewed

View reviewed changes

src/plugins/intel_cpu/src/nodes/kernels/jit_kernel_base.hpp Outdated

+                      const size_t kDataTypeSize = sizeof(float);
+                      if (is_valid_isa(x64::avx512_core)) {
+                          assert(reg_mask.isOPMASK());
+                          vgatherdps(xmm_val, ptr[reg_addr + xmm_index * scale + disp]);

Contributor

nshchego Oct 3, 2022

Why the mask is not used here?

lohika-vmostovy Nov 3, 2022

uni_vgatherdps and uni_vscatterdps were excluded from this PR as you requested.

nshchego reviewed

View reviewed changes

src/plugins/intel_cpu/src/nodes/kernels/jit_kernel_base.hpp Outdated

+                          assert(reg_mask.isYMM());
+                          Xbyak::Ymm ymm_mask{reg_mask.getIdx()};
+                          vgatherdps(xmm_val, ptr[reg_addr + xmm_index * scale + disp], ymm_mask);
+                      } else {

Contributor

nshchego Oct 3, 2022

What about AVX?

lohika-vmostovy Nov 3, 2022

vgatherdps was added since AVX2. For AVX it's emulated the same way as for SSE.

lohika-vmostovy Nov 3, 2022

uni_vgatherdps and uni_vscatterdps were excluded from this PR as you requested.

nshchego reviewed

View reviewed changes

src/plugins/intel_cpu/src/nodes/kernels/jit_kernel_base.hpp Outdated

+                          for (int i = 0; i < static_cast<int>(kSimdWidth); i++) {
+                              Xbyak::Label gather_end;
+                              uni_vpextrd(mask, xmm_mask, i);
+                              cmp(mask, 0xFFFFFFFF);

Contributor

nshchego Oct 3, 2022

Actually need to check just most significant bit.

lohika-vmostovy Nov 3, 2022

uni_vgatherdps and uni_vscatterdps were excluded from this PR as you requested.

nshchego reviewed

View reviewed changes

src/plugins/intel_cpu/src/nodes/kernels/jit_kernel_base.hpp Outdated

+                      }
+                  }
+                  inline void uni_vscatterdps(const Xbyak::Reg64& reg_addr,

Contributor

nshchego Oct 3, 2022

The same remarks as for the gather.

Also please move both of this function into PR where they are used.

lohika-vmostovy Nov 3, 2022

uni_vgatherdps and uni_vscatterdps were excluded from this PR as you requested.

rbaranchuk-capgemini reviewed

View reviewed changes

src/plugins/intel_cpu/src/nodes/kernels/registers_pool.hpp

rbaranchuk-capgemini reviewed

View reviewed changes

src/common/util/include/openvino/util/common_util.hpp Outdated

ceciliapeng2011 reviewed

View reviewed changes

src/plugins/intel_cpu/src/nodes/kernels/jit_kernel_base.hpp Outdated

ceciliapeng2011 reviewed

View reviewed changes

src/plugins/intel_cpu/src/nodes/kernels/jit_kernel_base.hpp Outdated

+                      return x64::is_subset(isa, max_cpu_isa_) && x64::mayiuse(isa);
+                  }
+                  inline void uni_vgatherdps(const Xbyak::Xmm &xmm_val,

Contributor

ceciliapeng2011 Oct 8, 2022 •

edited

Loading

How do you think of @dmitry-gorokhov 's suggestion in openvinotoolkit/oneDNN#145 to use jit-emitter instead for gather and scatter?
@lohika-denis-kotov @nshchego

lohika-vmostovy Nov 3, 2022

uni_vgatherdps() is not a part of this PR now

lohika-denis-kotov changed the title ~~[CPU] Implementation of jit_kernel_base~~ [WIP][CPU] Implementation of jit_kernel_base

lohika-denis-kotov force-pushed the feature/jit_kernel_base_class branch 2 times, most recently from 52309f8 to 7f94f4f Compare

October 11, 2022 10:36

lohika-denis-kotov force-pushed the feature/jit_kernel_base_class branch 2 times, most recently from 3856b51 to 6e3e679 Compare

October 11, 2022 11:52

avoskoboinyk-lohika suggested changes

View reviewed changes

src/plugins/intel_cpu/src/nodes/kernels/jit_base.hpp Outdated

src/plugins/intel_cpu/src/nodes/kernels/jit_base.hpp Outdated

lohika-denis-kotov force-pushed the feature/jit_kernel_base_class branch from 6e3e679 to cd6d806 Compare

October 11, 2022 19:40

lohika-denis-kotov changed the title ~~[WIP][CPU] Implementation of jit_kernel_base~~ [CPU] Implementation of jit_kernel_base

t-jankowski removed request for a team

October 12, 2022 08:18

lohika-denis-kotov mentioned this pull request

[Outdated, see #13790][CPU] StackAllocator for JIT kernels #13132

Closed

maxnick reviewed

View reviewed changes

src/plugins/intel_cpu/src/nodes/kernels/jit_kernel_base.hpp Outdated

src/plugins/intel_cpu/src/nodes/kernels/jit_base.hpp Outdated

Comment on lines +10 to +11

		#include <math.h>
		#include <dnnl_extension_utils.h>

Contributor

maxnick Oct 18, 2022

Do we really need those two header here?

lohika-vmostovy Nov 3, 2022

removed

src/plugins/intel_cpu/src/nodes/kernels/jit_kernel.hpp Outdated

Comment on lines +10 to +11

		#include <math.h>
		#include <dnnl_extension_utils.h>

Contributor

maxnick Oct 18, 2022

Could you please just check headers that are not used within the PR?

lohika-vmostovy Nov 3, 2022

done

src/plugins/intel_cpu/src/nodes/kernels/jit_kernel_base.hpp Outdated

Comment on lines +182 to +56

		RegistersPool::Ptr registers_pool_;
		std::unique_ptr<StackAllocator> stack_allocator_;

Contributor

maxnick Oct 18, 2022

As far as I remember we discussed placing those two entities on the jit_base level.

lohika-vmostovy Nov 3, 2022

Please, take one more look at this file and confirm that we really need to do this.
This change will additionally require to make stack allocator and registers pool members protected or to create two additional protected methods to create/destroy these objects.

src/plugins/intel_cpu/src/nodes/kernels/jit_kernel_base.hpp Outdated

+                          assert(xmm_index.getKind() == xmm_mask.getKind());
+                          std::vector<Xbyak::Reg> not_available_reg{reg_addr};
+                          const Xbyak::Reg64 idx = register_pool()->getInplaceFree<Xbyak::Reg64>(not_available_reg);

Contributor

maxnick Oct 18, 2022

The idea is to add factory methods to the jit_base level, so that we can hide the whole register management mechanism to decouple user code from the utility entities.

lohika-vmostovy Nov 4, 2022

What do you mean by decouple user code from the utility entities? You will still need to include registers pool header everywhere you are using register_pool(), no matter if it's located in jit_kernel_base or jit_base. Registers pool lifetime is already encapsulated in jit_kernel_base, so what's the point?

src/plugins/intel_cpu/src/nodes/kernels/registers_pool.hpp

                   }
+                  template<typename TReg>
+                  inline TReg getInplaceFree(std::vector<Xbyak::Reg>& not_available) const {

Contributor

maxnick Oct 18, 2022 •

edited

Loading

In my perception, RegisterPool should remain the bare minimum of the interface functionality in spite of simplicity, but the interface should be complete (as it is so far). Such utility code should be placed in jit_base.

lohika-vmostovy Nov 4, 2022

Agree. But getInplaceFree() is no longer a part of this PR.

nshchego reviewed

View reviewed changes

src/plugins/intel_cpu/src/nodes/kernels/jit_kernel_base.hpp Outdated

+              namespace ov {
+              namespace intel_cpu {
+              using namespace dnnl::impl;

Contributor

nshchego Oct 19, 2022

Please don't use 'using namespace' in header files.

lohika-vmostovy Nov 4, 2022

done

nshchego reviewed

View reviewed changes

src/plugins/intel_cpu/src/nodes/kernels/jit_kernel_traits.hpp Outdated

+              template<x64::cpu_isa_t isa, ov::element::Type_t element_type>
+              class jit_kernel_traits {
+              public:
+                  using Vmm = typename conditional3<isa == x64::sse41,

Contributor

nshchego Oct 19, 2022

Please add case for AVX as well

lohika-vmostovy Nov 4, 2022

Regarding using Vmm definition, there are about 90 occurrences of it in our code base. My understanding is that is more convenient to just use something like using Vmm = typename x64::cpu_isa_traits<isa>::Vmm instead.

SIMD_WIDTH is commonly used name/definition in our code base and I believe it's hard to find kernel which doesn't use something like it. However, in this header this name depends on ov::element::Type_t, which makes this definition too specific.

So, I decided to exclude this file from PR.
VCMPPS_LE and VCMPPS_GT constants were moved to jit_base. More constants like these will be added as needed.

nshchego reviewed

View reviewed changes

src/plugins/intel_cpu/src/nodes/kernels/jit_kernel_traits.hpp Outdated

+                  static constexpr unsigned VCMPPS_LE = 0x02;
+                  static constexpr unsigned VCMPPS_GT = 0x0e;
+                  static constexpr unsigned SIMD_WIDTH = x64::cpu_isa_traits<isa>::vlen / sizeof(typename ov::element_type_traits<element_type>::value_type);

Contributor

nshchego Oct 19, 2022

SIMD_WIDTH little bit confuses. The SIMD width is constant. Better to call it like ELEMENTS_PER_VECTOR.

lohika-vmostovy Nov 4, 2022

This file is no longer a part of this PR

nshchego reviewed

View reviewed changes

src/plugins/intel_cpu/src/nodes/kernels/jit_kernel_traits.hpp Outdated

+              namespace x64 = dnnl::impl::cpu::x64;
+              template<x64::cpu_isa_t isa, ov::element::Type_t element_type>
+              class jit_kernel_traits {

Contributor

nshchego Oct 19, 2022

Could you add some usage examples?

lohika-vmostovy Nov 4, 2022

This file is no longer a part of this PR

nshchego reviewed

View reviewed changes

src/plugins/intel_cpu/src/nodes/kernels/jit_base.hpp

+                  }
+              protected:
+                  x64::cpu_isa_t max_cpu_isa_;

Contributor

nshchego Oct 19, 2022

Please don't use such style "name_".

lohika-vmostovy Nov 4, 2022

Please, let me know which style has to be used here. Should I add m_ prefix or just use camel case? I believe you would expect camel case, but in that case there will be no indication of class member which is too unusual to me, so I would like to have your clarification.

avoskoboinyk-lohika approved these changes

View reviewed changes

lohika-denis-kotov and others added 4 commits

November 21, 2022 16:09


          Restore jit(_kernel)_base hierarchy

ad31a21


          Modify jit_kernel_base for gatherdd() functions to deduplicate some code

2f31d2b


          Drop emu_vgatherdps and fix a TODO inside gatherdd function

ba0d9ba


          Merge branch 'master' into feature/jit_kernel_base_class

5ccd47a

Collaborator

akladiev commented May 11, 2023

This PR will be closed in 2 weeks in case of no activity.

Collaborator

akladiev commented May 23, 2023

This PR was closed because it has been stalled for 2 week with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ceciliapeng2011 ceciliapeng2011 left review comments

maxnick maxnick left review comments

nshchego nshchego left review comments

akuporos Awaiting requested review from akuporos akuporos was automatically assigned from openvinotoolkit/openvino-configuration-mgmt

artanokhov Awaiting requested review from artanokhov artanokhov was automatically assigned from openvinotoolkit/openvino-configuration-mgmt

kblaszczak-intel Awaiting requested review from kblaszczak-intel kblaszczak-intel was automatically assigned from openvinotoolkit/openvino-docs-maintainers

ilya-lavrenov Awaiting requested review from ilya-lavrenov ilya-lavrenov was automatically assigned from openvinotoolkit/openvino-maintainers

ilyachur Awaiting requested review from ilyachur ilyachur was automatically assigned from openvinotoolkit/openvino-maintainers

tomdol Awaiting requested review from tomdol tomdol was automatically assigned from openvinotoolkit/openvino-maintainers

+3 more reviewers

lohika-vmostovy lohika-vmostovy left review comments

rbaranchuk-capgemini rbaranchuk-capgemini left review comments

avoskoboinyk-lohika avoskoboinyk-lohika approved these changes

Labels

category: CPU Stale