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.

Sign up for GitHub

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

Jump to bottom

benchdnn: introduce --bia-dt option for drivers supporting bias (fixed MFDNN-3031, fixed MFDNN-12936) #2335

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

benchdnn: introduce --bia-dt option for drivers supporting bias (fixed MFDNN-3031, fixed MFDNN-12936) #2335

wants to merge 12 commits into from
+1,263 −1,147

Conversation

dzarukin
Copy link
Contributor

@dzarukin dzarukin commented Jan 2, 2025 •
edited
Loading

MFDNN-3031
MFDNN-12936

To keep the compatibility, the default bias data type is undef, which means no bias is specified. This is aligned with current FWD_D and FWD_I settings. FWD_B value will update the bia-dt value to f32 which is also compatible with previous behavior.

At some point in v3.8 both FWD_B and BWD_WB values for the --dir option will be marked as deprecated and issue the corresponding warning.

To specify some data type for bias, FWD_D or FWD_I should be used:

benchdnn --dt=f16 --bia-dt=f16 --dir=FWD_I <desc>

Feel free to ignore matmul input files changes as it's a straight rename from --bia_dt to --bia-dt.

@dzarukin dzarukin requested review from a team as code owners January 2, 2025 23:15
@github-actions github-actions bot added documentation A request to change/fix/improve the documentation. Codeowner: @oneapi-src/onednn-doc platform:cpu-x64 Intel64/AMD64 processors. Codeowner: @oneapi-src/onednn-cpu-x64 component:tests Codeowner: @oneapi-src/onednn-arch labels Jan 2, 2025
@dzarukin
Copy link
Contributor Author

dzarukin commented Jan 2, 2025

make test

rjoursler
rjoursler reviewed Jan 2, 2025
View reviewed changes
tests/benchdnn/doc/driver_matmul.md
@@ -21,7 +21,7 @@ where *matmul-knobs* are:
specification for `src`, `weights`, and `dst` tensors through
strides values. Refer to [option documentation](knob_strides.md)
for details.
- `--bia_dt={undef [default], f32, s32, s8, u8}` -- bias data type.
- `--bia-dt={undef [default], f32, s32, s8, u8}` -- bias data type.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any particular reason we aren't just fully spelling out this argument as --bias-dt? Shortening bias to bia just seems like it creates naming complexity for minimal benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The path of respecting the legacy... :)
But I'm fine to rename it in case we get some up votes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we historically used fully spelled names in API (except for src/dst for some reason), and 3 letter shortening in benchdnn (src/dst/wei/bia). So unless we want to fully spell other names as well in benchdnn (e.g. wei -> weights), I would vote to keep it as is.

densamoilov
densamoilov approved these changes Jan 3, 2025
View reviewed changes
tests/benchdnn/conv/bench_conv.cpp Outdated Show resolved Hide resolved
tests/benchdnn/doc/driver_ip.md Outdated Show resolved Hide resolved
@dzarukin dzarukin force-pushed the dzarukin/benchdnn_bia_dt branch from d766266 to befeb28 Compare January 4, 2025 00:42
@dzarukin dzarukin requested a review from a team as a code owner January 4, 2025 00:42
@github-actions github-actions bot added the component:graph-api Codeowner: @oneapi-src/onednn-graph label Jan 4, 2025
@dzarukin
Copy link
Contributor Author

dzarukin commented Jan 4, 2025

make test

ranukund
ranukund approved these changes Jan 6, 2025
View reviewed changes
Copy link
Contributor

@ranukund ranukund left a comment

Choose a reason for hiding this comment

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

The .md files look good to me, thanks!

@dzarukin dzarukin force-pushed the dzarukin/benchdnn_bia_dt branch from befeb28 to c1228bd Compare January 8, 2025 18:32
@dzarukin dzarukin changed the title benchdnn: introduce --bia-dt option for drivers supporting bias (fixed MFDNN-3031) benchdnn: introduce --bia-dt option for drivers supporting bias (fixed MFDNN-3031, fixed MFDNN-12936) Jan 8, 2025
@dzarukin
Copy link
Contributor Author

dzarukin commented Jan 8, 2025

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_conv
enable benchdnn_deconv
enable benchdnn_ip

@dzarukin dzarukin force-pushed the dzarukin/benchdnn_bia_dt branch from 3100fd1 to 4d6c004 Compare January 9, 2025 00:03
@dzarukin
Copy link
Contributor Author

dzarukin commented Jan 9, 2025

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_conv
enable benchdnn_deconv
enable benchdnn_ip

@dzarukin dzarukin force-pushed the dzarukin/benchdnn_bia_dt branch 2 times, most recently from 752361e to fa70bfb Compare January 10, 2025 00:04
@dzarukin
Copy link
Contributor Author

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_conv
enable benchdnn_deconv
enable benchdnn_ip

@dzarukin dzarukin force-pushed the dzarukin/benchdnn_bia_dt branch from fa70bfb to 25804ee Compare January 10, 2025 00:36
@dzarukin dzarukin force-pushed the dzarukin/benchdnn_bia_dt branch from 25804ee to c907431 Compare January 10, 2025 04:18
@dzarukin
Copy link
Contributor Author

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_conv
enable benchdnn_deconv
enable benchdnn_ip

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

@atkassen atkassen atkassen left review comments

@rjoursler rjoursler rjoursler left review comments

@ranukund ranukund ranukund approved these changes

@mgouicem mgouicem mgouicem approved these changes

@densamoilov densamoilov densamoilov approved these changes

Assignees
No one assigned
Labels
component:graph-api Codeowner: @oneapi-src/onednn-graph component:tests Codeowner: @oneapi-src/onednn-arch documentation A request to change/fix/improve the documentation. Codeowner: @oneapi-src/onednn-doc platform:cpu-x64 Intel64/AMD64 processors. Codeowner: @oneapi-src/onednn-cpu-x64
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dzarukin @densamoilov @mgouicem @rjoursler @atkassen @ranukund