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

Refactor convolution test suite #24

Merged
merged 16 commits into from
Oct 30, 2024

Conversation

erman-gurses
Copy link
Contributor

@erman-gurses erman-gurses commented Sep 10, 2024

Progress on #2.
This PR refactors, adds, and migrates conv2d e2e tests from the PRs here: iree-org/iree#16849 and iree-org/iree#18125

linalg_ops/convolution/CMakeLists.txt Show resolved Hide resolved
linalg_ops/convolution/generate_e2e_conv2d_tests.py Outdated Show resolved Hide resolved
linalg_ops/convolution/generate_e2e_conv2d_tests.py Outdated Show resolved Hide resolved
linalg_ops/convolution/generate_e2e_conv2d_tests.py Outdated Show resolved Hide resolved
linalg_ops/convolution/generate_e2e_conv2d_tests.py Outdated Show resolved Hide resolved
linalg_ops/convolution/generate_e2e_conv2d_tests.py Outdated Show resolved Hide resolved
linalg_ops/convolution/generate_e2e_conv2d_tests.py Outdated Show resolved Hide resolved
linalg_ops/convolution/generate_e2e_conv2d_tests.py Outdated Show resolved Hide resolved
linalg_ops/CMakeLists.txt Outdated Show resolved Hide resolved
linalg_ops/iree-e2e-conv2d-test.cc Outdated Show resolved Hide resolved
Signed-off-by: erman-gurses <[email protected]>
Signed-off-by: erman-gurses <[email protected]>
Signed-off-by: erman-gurses <[email protected]>
@erman-gurses
Copy link
Contributor Author

@ScottTodd, the CPU part is done. Let me know if you have any input.

Copy link
Member

@ScottTodd ScottTodd left a comment

Choose a reason for hiding this comment

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

One comment for now. Haven't looked at the generate python script or the CI workflow logs yet.

linalg_ops/convolution/CMakeLists.txt Outdated Show resolved Hide resolved
linalg_ops/convolution/generate_test_mlir_files.sh Outdated Show resolved Hide resolved
linalg_ops/convolution/generate_test_mlir_files.sh Outdated Show resolved Hide resolved
@ScottTodd ScottTodd self-requested a review October 21, 2024 23:56
Copy link
Member

@ScottTodd ScottTodd left a comment

Choose a reason for hiding this comment

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

mostly LGTM, a few small comments

linalg_ops/test_utils.c Outdated Show resolved Hide resolved
linalg_ops/convolution/generate_e2e_conv2d_tests.py Outdated Show resolved Hide resolved
linalg_ops/convolution/generate_e2e_conv2d_tests.py Outdated Show resolved Hide resolved
linalg_ops/iree-e2e-conv2d-test.cc Outdated Show resolved Hide resolved
linalg_ops/iree-e2e-conv2d-test.cc Outdated Show resolved Hide resolved
linalg_ops/iree-e2e-conv2d-test.cc Outdated Show resolved Hide resolved
@erman-gurses erman-gurses marked this pull request as ready for review October 24, 2024 01:37
Signed-off-by: erman-gurses <[email protected]>
Signed-off-by: erman-gurses <[email protected]>
linalg_ops/test_utils.c Outdated Show resolved Hide resolved
linalg_ops/convolution/generate_test_mlir_files.sh Outdated Show resolved Hide resolved
Copy link

@Max191 Max191 left a comment

Choose a reason for hiding this comment

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

The formats for CPU tests and GPU tests are quite different, but they should be able to use the same source IR. Can we consolidate the test generation into a single format to reduce the number of generated tests?

It would be good to cover the following element type and layout combinations on both CPU and GPU to start:

  • Both NCHW and NHWC convs for all the following dtype combinations:
    • f16_f16_f16 (with and without winograd)
    • f32_f32_f32 (with and without winograd)
    • f16_f16_f32 (with and without winograd)
    • i8_i8_i32 (without winograd)

EDIT: Btw, I don't mean to block progress if this is just an initial step, but if we are merging these tests, then we should try to share the implementation. If you would prefer to do this as a follow up that is also okay.

linalg_ops/convolution/CMakeLists.txt Show resolved Hide resolved
linalg_ops/convolution/CMakeLists.txt Outdated Show resolved Hide resolved
linalg_ops/convolution/CMakeLists.txt Outdated Show resolved Hide resolved
linalg_ops/convolution/CMakeLists.txt Show resolved Hide resolved
linalg_ops/convolution/generate_test_mlir_files.sh Outdated Show resolved Hide resolved
linalg_ops/convolution/generate_test_mlir_files.sh Outdated Show resolved Hide resolved
@erman-gurses erman-gurses requested a review from Max191 October 29, 2024 22:11
Copy link

@Max191 Max191 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

Signed-off-by: erman-gurses <[email protected]>
Signed-off-by: erman-gurses <[email protected]>
Signed-off-by: erman-gurses <[email protected]>
@erman-gurses erman-gurses changed the title Add convolution test suite Refactor convolution test suite Oct 30, 2024
@erman-gurses erman-gurses merged commit 15315f5 into iree-org:main Oct 30, 2024
2 checks passed
@erman-gurses erman-gurses deleted the add-conv-test-suite branch October 30, 2024 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants