Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Snippets] Brgemm blocking by KN dims at the LIR level #19335

Conversation

v-Golubev
Copy link
Contributor

@v-Golubev v-Golubev commented Aug 22, 2023

Details:

  • Brgemm KN blocking removed from BrgemmCPU emitter
  • Brgemm blocking implemented on Linear IR level
  • InsertTailLoop rework: added a mechanism for subtensor propagation via shape inference infrastructure
  • dim_idx moved from LoopInfo to LoopPort
  • Several fixes

Tickets:

  • 115164
  • 112122

Prerequisites:

@github-actions github-actions bot added category: IE Tests OpenVINO Test: plugins and common category: CPU OpenVINO CPU plugin labels Aug 22, 2023
@v-Golubev v-Golubev force-pushed the vg/snippets/mha/lir_kn_blocking branch 2 times, most recently from 3ac018c to 20b2a91 Compare August 24, 2023 15:39
@v-Golubev v-Golubev force-pushed the vg/snippets/mha/lir_kn_blocking branch 2 times, most recently from 73ae818 to ef6b38b Compare September 4, 2023 15:44
@v-Golubev v-Golubev force-pushed the vg/snippets/mha/lir_kn_blocking branch from ef6b38b to 11a2a01 Compare September 11, 2023 17:05
@github-actions github-actions bot removed the category: IE Tests OpenVINO Test: plugins and common label Sep 11, 2023
@v-Golubev v-Golubev force-pushed the vg/snippets/mha/lir_kn_blocking branch 2 times, most recently from cf8a1de to 9defd95 Compare September 12, 2023 14:20
@v-Golubev v-Golubev marked this pull request as ready for review September 12, 2023 17:27
@v-Golubev v-Golubev requested review from a team as code owners September 12, 2023 17:27
@v-Golubev v-Golubev force-pushed the vg/snippets/mha/lir_kn_blocking branch from 9defd95 to 3f554be Compare September 12, 2023 17:27
@v-Golubev v-Golubev force-pushed the vg/snippets/mha/lir_kn_blocking branch 2 times, most recently from 3e052a2 to d027a05 Compare September 29, 2023 12:24
@v-Golubev v-Golubev requested review from a team as code owners September 29, 2023 12:24
@github-actions github-actions bot added the category: IE Tests OpenVINO Test: plugins and common label Sep 29, 2023
@v-Golubev v-Golubev force-pushed the vg/snippets/mha/lir_kn_blocking branch 4 times, most recently from 51a567a to 85c7b3c Compare October 4, 2023 10:57
@v-Golubev v-Golubev force-pushed the vg/snippets/mha/lir_kn_blocking branch from d7199f7 to 6b0346a Compare October 9, 2023 07:42
@v-Golubev v-Golubev added this to the 2023.2 milestone Oct 9, 2023
@v-Golubev v-Golubev force-pushed the vg/snippets/mha/lir_kn_blocking branch from 2c7622b to 0fda262 Compare November 21, 2023 17:29
@v-Golubev
Copy link
Contributor Author

@IvanNovoselov @a-sidorova as we agreed offline, I also separated snippets operation Buffer on 2 ones. Could you please take a look on the 2 latest commits (AMX scratchpad buffer is moved inside blocking loops and Buffer operation separation)? Thanks in advance

@v-Golubev
Copy link
Contributor Author

It was decided to merge the PR after #19644

@v-Golubev
Copy link
Contributor Author

@dmitry-gorokhov could you please take a look?

@v-Golubev v-Golubev force-pushed the vg/snippets/mha/lir_kn_blocking branch from 68b48a0 to 2699590 Compare December 5, 2023 13:11
Copy link
Contributor

@dmitry-gorokhov dmitry-gorokhov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No major concerns
@v-Golubev Please answer remaining questions

* @param ptr_increments specifies i/o pointer increment performed on every iteration. This is an alternative to
* apply_increments, which enables more flexibility.
* @param is_incremented describes which data pointers attributed to the loop should be incremented on every iteration.
* @param ptr_increments specifies i/o pointer increment performed on every iteration if the following is_incremented[i] is true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering why is_incremented[i] cannot be expressed with ptr_increments[i] == 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it can be expressed. Need to revert this change - I will do that in follow up PRs

Copy link
Contributor

@a-sidorova a-sidorova Dec 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it might be useful for dynamic loops (for dynamic loop end emitter) when you need this port as LoopPort but it shouldn't be incremented.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it might be useful for dynamic loops (for dynamic loop end emitter) when you need this port as LoopPort but it shouldn't be incremented.

Lets refactor in bounds of DynamicLoop enablement then.

@@ -32,19 +32,19 @@ class BrgemmCPU : public snippets::op::Brgemm {
BrgemmCPU(const Output<Node>& A, const Output<Node>& B, const Type type,
const size_t offset_a = 0, const size_t offset_b = 0, const size_t offset_c = 0,
std::vector<size_t> layout_a = {}, std::vector<size_t> layout_b = {}, std::vector<size_t> layout_c = {},
const size_t blk_size_m = 0, const size_t blk_size_k = 0, const size_t blk_size_n = 0);
const size_t blk_size_m = 0, const size_t blk_size_k = 0, const size_t blk_size_n = 0, const float beta = 0.f);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't analyzed the code in details, but was is the purpose of having blk size as part of Brgemm semantics?
It sounds like everything should be done with transformations now. As far as I remember blocking heuristics are also expressed with separate transformation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blk size parameters are used to propagate information about preferred block sizes between ov::Model transformation (which applies blocking heuristics) and LIR blocking markup transformation.
We have already discussed this topic with @IvanNovoselov: we came to the conclusion that these 2 transformations can be easily combined into one: in this case, there will be no need to store block sizes as node fields.

@dmitry-gorokhov dmitry-gorokhov merged commit 261cf4e into openvinotoolkit:master Dec 8, 2023
73 checks passed
akuporos pushed a commit to akuporos/openvino that referenced this pull request Dec 8, 2023
github-merge-queue bot pushed a commit that referenced this pull request Feb 27, 2024
### Details:
- *Introduced loop handlers which define how to handle first/last loop
iteration*
- *Changed loop markup logic: loops with `increment > work_amount` are
not created anymore: in this case `increment` is set equal to
`work_amount`*

### Tickets:
 - *CVS-119851*

### Prerequisites:
- #19335
- #21382
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: CPU OpenVINO CPU plugin category: IE Tests OpenVINO Test: plugins and common
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants