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

[CK API] 2d backward data convolution composable kernel integration #1874

Merged
merged 15 commits into from
Jan 3, 2023
Merged

[CK API] 2d backward data convolution composable kernel integration #1874

merged 15 commits into from
Jan 3, 2023

Conversation

iq136boy
Copy link
Contributor

  1. Integrated 2d backward data convolution composable kernel
  2. Adding test cases.

@iq136boy iq136boy self-assigned this Nov 10, 2022
JehandadKhan
JehandadKhan requested changes Nov 10, 2022
View reviewed changes
test/CMakeLists.txt Show resolved Hide resolved
@averinevg
Copy link
Collaborator

@atamazov Please review

@iq136boy iq136boy requested review from atamazov, averinevg and bghimireamd and removed request for atamazov December 12, 2022 16:08
@junliume junliume merged commit ebcbc4b into develop Jan 3, 2023
@atamazov

This comment was marked as duplicate.

Sign in to view
@atamazov atamazov mentioned this pull request Jan 9, 2023
Open
5 tasks
@junliume
Copy link
Collaborator

@iq136boy Were these important comments resolved? I don't see the "resolved" marks (usually I do not look into code changes until a comment is masked as "resolved")

/cc @junliume

ping @iq136boy

atamazov
atamazov reviewed Jan 13, 2023
View reviewed changes
Copy link
Contributor

@atamazov atamazov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tests do not guarantee verification of a test the new solver (#1874 (review))

test/CMakeLists.txt
COMMAND $<TARGET_FILE:test_conv2d> ${MIOPEN_TEST_FLOAT_ARG} --disable-backward-data --disable-backward-weights --verbose --input 256 256 56 56 --weights 256 64 1 1 --output_type fp16 --in_layout NHWC --fil_layout NHWC --out_layout NHWC --pads_strides_dilations 0 0 1 1 1 1
COMMAND $<TARGET_FILE:test_conv2d> ${MIOPEN_TEST_FLOAT_ARG} --disable-forward --disable-backward-weights --verbose --input 256 128 28 28 --weights 128 128 3 3 --output_type fp32 --in_layout NHWC --fil_layout NHWC --out_layout NHWC --pads_strides_dilations 1 1 1 1 1 1
COMMAND $<TARGET_FILE:test_conv2d> ${MIOPEN_TEST_FLOAT_ARG} --disable-forward --disable-backward-weights --verbose --input 128 512 7 7 --weights 512 512 3 3 --output_type fp32 --in_layout NHWC --fil_layout NHWC --out_layout NHWC --pads_strides_dilations 1 1 1 1 1 1
COMMAND $<TARGET_FILE:test_conv2d> ${MIOPEN_TEST_FLOAT_ARG} --disable-forward --disable-backward-weights --verbose --input 128 64 56 56 --weights 64 64 1 1 --output_type fp32 --in_layout NHWC --fil_layout NHWC --out_layout NHWC --pads_strides_dilations 0 0 1 1 1 1
Copy link
Contributor

Choose a reason for hiding this comment

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

@iq136boy Please note that these tests are not guaranteed to test CK kernels. The reason is that the library selects the "fastest known" kernels or use Immediate Mode Fallback heuristics to select "likely the best" kernel, and therefore we do not know which Solver will be used. Please employ MIOPEN_FIND_MODE=normal and MIOPEN_DEBUG_FIND_ONLY_SOLVER=... in order to limit library to work with a known set of Solvers (if you need).

test/CMakeLists.txt Show resolved Hide resolved
atamazov
atamazov reviewed Jan 13, 2023
View reviewed changes
test/CMakeLists.txt
COMMAND $<TARGET_FILE:test_conv2d> ${MIOPEN_TEST_FLOAT_ARG} --disable-backward-data --disable-backward-weights --verbose --input 256 128 28 28 --weights 128 128 3 3 --output_type fp16 --in_layout NHWC --fil_layout NHWC --out_layout NHWC --pads_strides_dilations 1 1 1 1 1 1
COMMAND $<TARGET_FILE:test_conv2d> ${MIOPEN_TEST_FLOAT_ARG} --disable-backward-data --disable-backward-weights --verbose --input 128 512 7 7 --weights 512 512 3 3 --output_type fp16 --in_layout NHWC --fil_layout NHWC --out_layout NHWC --pads_strides_dilations 1 1 1 1 1 1
COMMAND $<TARGET_FILE:test_conv2d> ${MIOPEN_TEST_FLOAT_ARG} --disable-backward-data --disable-backward-weights --verbose --input 128 64 56 56 --weights 64 64 1 1 --output_type fp16 --in_layout NHWC --fil_layout NHWC --out_layout NHWC --pads_strides_dilations 0 0 1 1 1 1
COMMAND $<TARGET_FILE:test_conv2d> ${MIOPEN_TEST_FLOAT_ARG} --disable-backward-data --disable-backward-weights --verbose --input 256 256 56 56 --weights 256 64 1 1 --output_type fp16 --in_layout NHWC --fil_layout NHWC --out_layout NHWC --pads_strides_dilations 0 0 1 1 1 1
Copy link
Contributor

Choose a reason for hiding this comment

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

This config is invalid. Silently skipped due to #1934

atamazov
atamazov reviewed Jan 14, 2023
View reviewed changes
Copy link
Contributor

@atamazov atamazov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

[tests] The added tests only test the new solver with INT8 input datatype. But this is useless because INT8 is only supported for Forward convolutions. Testcase is silently skipped due to #1934.

@atamazov atamazov mentioned this pull request Jan 17, 2023
Open
@iq136boy
Copy link
Contributor Author

@iq136boy Were these important comments resolved? I don't see the "resolved" marks (usually I do not look into code changes until a comment is masked as "resolved")

/cc @junliume

ping @iq136boy

@junliume All the comments has been addressed in the codes. I have mark these conversation as 'resolved'.

@junliume junliume deleted the dfeng/ck_conv_bwd_data branch March 23, 2023 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atamazov atamazov atamazov requested changes

@JehandadKhan JehandadKhan JehandadKhan approved these changes

@junliume junliume junliume approved these changes

@averinevg averinevg averinevg approved these changes

@bghimireamd bghimireamd bghimireamd approved these changes

Assignees

@iq136boy iq136boy

Labels
TESTING_CI_PASSED
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@iq136boy @averinevg @atamazov @junliume @JehandadKhan @bghimireamd