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

[WIP] src: cpu: x64: jit_brdgmm_dw_conw: make brdgmm support fusing sum op #2337

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

amakarev
Copy link
Contributor

@amakarev amakarev commented Jan 3, 2025

Brdgmm kernel could not be selected when using the --attr-post-ops=sum flag. jit_dw was used instead.
This PR makes it possible to use brdgmm kernel with sum post-op

The changes look trivial, In fact, there was already support, but brdgmm could not be dispatched because the function(has_default_values) signature was updated a long time ago but the function call was not updated. As a result, data_type_undef was always used inside, which prevented the brdgmm kernel from being selected

@amakarev amakarev added the platform:cpu-x64 Intel64/AMD64 processors. Codeowner: @oneapi-src/onednn-cpu-x64 label Jan 3, 2025
@amakarev amakarev requested a review from a team as a code owner January 3, 2025 12:23
@amakarev amakarev force-pushed the amakarev/brdgmm-support-fusing-sum-op branch from 5d7c753 to d4a0ca9 Compare January 3, 2025 13:59
Copy link
Contributor

@inteldimitrius inteldimitrius left a comment

Choose a reason for hiding this comment

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

LGTM

@dzarukin
Copy link
Contributor

dzarukin commented Jan 3, 2025

dst_dt argument is responsible to support s8 sum post-op while dst data type is u8, or vice versa. It doesn't have anything about enabling the sum post-op per se. undef data type passed requires sum post-op to check if it uses exactly the same data type as destination tensor, not allowing divergence from the first sentence.

It would be nice to see the following:

  • There's actually some coverage for brdgmm + sum post+op in ci (or some nightly) file.
  • The support flipped from jit to brgemm for a regular sum post-op case (same data type) and for a sum_dt != dst_dt (updating description with verbose before and after should be fine).

@amakarev
Copy link
Contributor Author

amakarev commented Jan 3, 2025

dst_dt argument is responsible to support s8 sum post-op while dst data type is u8, or vice versa. It doesn't have anything about enabling the sum post-op per se. undef data type passed requires sum post-op to check if it uses exactly the same data type as destination tensor, not allowing divergence from the first sentence.

It would be nice to see the following:

  • There's actually some coverage for brdgmm + sum post+op in ci (or some nightly) file.
  • The support flipped from jit to brgemm for a regular sum post-op case (same data type) and for a sum_dt != dst_dt (updating description with verbose before and after should be fine).

Got it, thanks for the clarification

@amakarev amakarev changed the title src: cpu: x64: jit_brdgmm_dw_conw: make brdgmm support fusing sum op [WIP] src: cpu: x64: jit_brdgmm_dw_conw: make brdgmm support fusing sum op Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform:cpu-x64 Intel64/AMD64 processors. Codeowner: @oneapi-src/onednn-cpu-x64
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants