[CPU]fix offset in store emitter issue #26083

tiger100256-hu · 2024-08-15T23:59:18Z

Details:

offset in store emitter is offset of dst gpr register, and should parse together with reg_dst

Tickets:

CVS-149693

src/plugins/intel_cpu/src/nodes/interpolate.cpp

src/plugins/intel_cpu/src/emitters/plugin/x64/jit_emitter.cpp

chenhu-wang · 2024-08-20T03:11:35Z

Please help change
const int offset = in_idxs.size() == 2 ? in_idxs[1] : 0;
to
const int offset = out_idxs.size() == 2 ? out_idxs[1] : 0;
in https://github.com/openvinotoolkit/openvino/blob/master/src/plugins/intel_cpu/src/emitters/plugin/x64/jit_load_store_emitters.cpp#L679

to fix the bug. Thanks!

chenhu-wang · 2024-08-21T06:14:46Z

@a-sidorova , for store emitter, the offset of dst gpr is passed with input vec reg. This seems like confusing and not friendly to use, could error prone, such as this change:#22816. We moved the offset as part of dst gpr. Could you please taka a look? Thanks!

chenhu-wang · 2024-08-21T06:16:37Z

src/plugins/intel_cpu/src/emitters/plugin/x64/jit_load_store_emitters.cpp

@@ -676,7 +676,7 @@ void jit_store_emitter::emit_data() const {
 }

 void jit_store_emitter::emit_impl(const std::vector<size_t> &in_idxs, const std::vector<size_t> &out_idxs) const {
-    const int offset = in_idxs.size() == 2 ? in_idxs[1] : 0;
+    const int offset = out_idxs.size() == 2 ? out_idxs[1] : 0;


Could you please also add a description regarding offset for store and load emitter?

Agree. Otherwise this is sacred knowledge which is known by a few developers

a-sidorova · 2024-08-21T13:24:32Z

@a-sidorova , for store emitter, the offset of dst gpr is passed with input vec reg. This seems like confusing and not friendly to use, could error prone, such as this change:#22816. We moved the offset as part of dst gpr. Could you please taka a look? Thanks!

As I remember, the purpose of using offset as input of store_emitter was to pass the offset to the emitter as the separate input emitter/node (for example, when the offset can be dynamic in snippets). This idea was proposed by @dmitry-gorokhov a few years ago.

@dmitry-gorokhov could you please share your opinion about moving of passing offset to output in store_emitter? In my opinion, the offset is attribute which is passed from another emitter/node/kernel. Because of that the offset is input parameter or me and should be passed in input indexes as before.

dmitry-gorokhov · 2024-08-21T18:41:17Z

@a-sidorova , for store emitter, the offset of dst gpr is passed with input vec reg. This seems like confusing and not friendly to use, could error prone, such as this change:#22816. We moved the offset as part of dst gpr. Could you please taka a look? Thanks!

As I remember, the purpose of using offset as input of store_emitter was to pass the offset to the emitter as the separate input emitter/node (for example, when the offset can be dynamic in snippets). This idea was proposed by @dmitry-gorokhov a few years ago.

@dmitry-gorokhov could you please share your opinion about moving of passing offset to output in store_emitter? In my opinion, the offset is attribute which is passed from another emitter/node/kernel. Because of that the offset is input parameter or me and should be passed in input indexes as before.

There are no strict requirements on emitters arguments placement. So we can adjust in any way if the solution considered msot convinient.
Initial idea was to aling basic emitters signature with usual ISA rules. E.g. 2 arguments of vmovups (dst, src). From this assumption vector mask should be associated with vector register and be part of inputs/outputs respectivly. Same about offset - it should be associated with the ptr.

The point about offset as input parameter is correct, but seems it correponds to another level of abstraction: execution graph. In the graph Store operation may obtain offset as an input if this is simplier way to express pointer arithmetic. On the other hand Emitter is an abstraction over ISA which might be used in different contexts, not only inside Snippets.
One of the possible solutions is to have separate adapter inside Snippets emitters scope which will remap parameters in desired way.

a-sidorova · 2024-08-22T05:42:50Z

src/plugins/intel_cpu/src/nodes/interaction.cpp

@@ -136,7 +136,7 @@ struct jit_move_scale_kernel : public jit_uni_move_scale_kernel, public jit_gene
            emitters[seed].reset(new jit_store_emitter(this, isa, src_prc, dst_prc, elt_num));
        }

-        emitters[seed]->emit_code({static_cast<size_t>(vmm_src.getIdx()), 0}, {static_cast<size_t>(reg_dst.getIdx())},
+        emitters[seed]->emit_code({static_cast<size_t>(vmm_src.getIdx())}, {static_cast<size_t>(reg_dst.getIdx()), 0},


Minor: offset = 0 is set by default in jit_store_emitter::emit_impl() so we don't need to manually pass zero everywhere

a-sidorova · 2024-08-22T05:45:26Z

src/plugins/intel_cpu/src/emitters/plugin/x64/jit_load_store_emitters.cpp

@@ -676,7 +676,7 @@ void jit_store_emitter::emit_data() const {
 }

 void jit_store_emitter::emit_impl(const std::vector<size_t> &in_idxs, const std::vector<size_t> &out_idxs) const {
-    const int offset = in_idxs.size() == 2 ? in_idxs[1] : 0;
+    const int offset = out_idxs.size() == 2 ? out_idxs[1] : 0;


Agree. Otherwise this is sacred knowledge which is known by a few developers

Signed-off-by: HU Yuan2 <[email protected]>

a-sidorova · 2024-08-22T11:50:17Z

src/plugins/intel_cpu/src/emitters/plugin/x64/jit_load_store_emitters.cpp

@@ -676,7 +677,8 @@ void jit_store_emitter::emit_data() const {
 }

 void jit_store_emitter::emit_impl(const std::vector<size_t> &in_idxs, const std::vector<size_t> &out_idxs) const {
-    const int offset = in_idxs.size() == 2 ? in_idxs[1] : 0;
+    // offset in store emitter is the offset of dst gpr register, should be parsed from out_ids.


Minor: The best place for this comment in the corresponding header file before class definitions 😃 Another developer will take a look at interface and won't find the description for offset. And only after searching in files, he will find this comment.

Signed-off-by: HU Yuan2 <[email protected]>

github-actions bot added the category: CPU OpenVINO CPU plugin label Aug 15, 2024

chenhu-wang requested changes Aug 20, 2024

View reviewed changes

src/plugins/intel_cpu/src/nodes/interpolate.cpp Outdated Show resolved Hide resolved

chenhu-wang reviewed Aug 20, 2024

View reviewed changes

src/plugins/intel_cpu/src/emitters/plugin/x64/jit_emitter.cpp Outdated Show resolved Hide resolved

src/plugins/intel_cpu/src/emitters/plugin/x64/jit_emitter.cpp Outdated Show resolved Hide resolved

chenhu-wang reviewed Aug 20, 2024

View reviewed changes

src/plugins/intel_cpu/src/emitters/plugin/x64/jit_emitter.cpp Outdated Show resolved Hide resolved

tiger100256-hu changed the title ~~fix store emitter in_idxs checking issue~~ [CPU]fix offset in store emitter issue Aug 20, 2024

tiger100256-hu marked this pull request as ready for review August 20, 2024 11:53

tiger100256-hu requested review from a team as code owners August 20, 2024 11:53

tiger100256-hu requested a review from chenhu-wang August 21, 2024 06:02

chenhu-wang approved these changes Aug 21, 2024

View reviewed changes

a-sidorova reviewed Aug 22, 2024

View reviewed changes

dmitry-gorokhov added this to the 2024.4 milestone Aug 22, 2024

tiger100256-hu added 6 commits August 22, 2024 16:03

fix store emitter in_idxs checking issue

a14d9db

Signed-off-by: HU Yuan2 <[email protected]>

put dst offset in out_ids in store emitter

f9c9066

Signed-off-by: HU Yuan2 <[email protected]>

skip check offset in in_idxs and out_idxs

586bf6c

Signed-off-by: HU Yuan2 <[email protected]>

the checking is right, don't need to modify it

9295a5a

Signed-off-by: HU Yuan2 <[email protected]>

remove offset = 0, which is set by emit_impl

f9fb181

Signed-off-by: HU Yuan2 <[email protected]>

add comments for offset in load and store emitter

90c460c

Signed-off-by: HU Yuan2 <[email protected]>

tiger100256-hu force-pushed the huyuan/store_emmitter branch from 450cdfb to 90c460c Compare August 22, 2024 10:37

a-sidorova approved these changes Aug 22, 2024

View reviewed changes

peterchen-intel assigned dmitry-gorokhov Aug 23, 2024

also add comment in header file

35d46b7

Signed-off-by: HU Yuan2 <[email protected]>

dmitry-gorokhov approved these changes Aug 23, 2024

View reviewed changes

dmitry-gorokhov enabled auto-merge August 23, 2024 10:55

Merge branch 'master' into huyuan/store_emmitter

7e064db

dmitry-gorokhov added this pull request to the merge queue Aug 26, 2024

Merged via the queue into openvinotoolkit:master with commit db44fb6 Aug 26, 2024
140 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CPU]fix offset in store emitter issue #26083

[CPU]fix offset in store emitter issue #26083

tiger100256-hu commented Aug 15, 2024 •

edited

Loading

chenhu-wang commented Aug 20, 2024

chenhu-wang commented Aug 21, 2024 •

edited

Loading

chenhu-wang Aug 21, 2024

a-sidorova Aug 22, 2024

a-sidorova commented Aug 21, 2024 •

edited

Loading

dmitry-gorokhov commented Aug 21, 2024 •

edited

Loading

a-sidorova Aug 22, 2024

a-sidorova Aug 22, 2024

a-sidorova Aug 22, 2024 •

edited

Loading

[CPU]fix offset in store emitter issue #26083

[CPU]fix offset in store emitter issue #26083

Conversation

tiger100256-hu commented Aug 15, 2024 • edited Loading

Details:

Tickets:

chenhu-wang commented Aug 20, 2024

chenhu-wang commented Aug 21, 2024 • edited Loading

chenhu-wang Aug 21, 2024

Choose a reason for hiding this comment

a-sidorova Aug 22, 2024

Choose a reason for hiding this comment

a-sidorova commented Aug 21, 2024 • edited Loading

dmitry-gorokhov commented Aug 21, 2024 • edited Loading

a-sidorova Aug 22, 2024

Choose a reason for hiding this comment

a-sidorova Aug 22, 2024

Choose a reason for hiding this comment

a-sidorova Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

tiger100256-hu commented Aug 15, 2024 •

edited

Loading

chenhu-wang commented Aug 21, 2024 •

edited

Loading

a-sidorova commented Aug 21, 2024 •

edited

Loading

dmitry-gorokhov commented Aug 21, 2024 •

edited

Loading

a-sidorova Aug 22, 2024 •

edited

Loading