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

Delete duplicated tests in iree/test/e2e/ #5446

Merged
merged 3 commits into from
Apr 19, 2021
Merged

Delete duplicated tests in iree/test/e2e/ #5446

merged 3 commits into from
Apr 19, 2021

Conversation

hanhanW
Copy link
Contributor

@hanhanW hanhanW commented Apr 15, 2021 •
edited
Loading

Also delete unnecessary flags in iree/test/e2e and iree/compiler/Dialect/HAL/Target/LLVM/test/smoketest.mlir

Part of #5442

@google-cla google-cla bot added the cla: yes label Apr 15, 2021
MaheshRavishankar
MaheshRavishankar requested changes Apr 15, 2021
View reviewed changes
Copy link
Contributor

@MaheshRavishankar MaheshRavishankar left a comment

Choose a reason for hiding this comment

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

Lets keep this for now. If you want you can create a new column on Linalg on tensors project, and add this PR to that

@hanhanW
Copy link
Contributor Author

hanhanW commented Apr 15, 2021

Lets keep this for now. If you want you can create a new column on Linalg on tensors project, and add this PR to that

The flag is not used. It now looks into the target backend to decide whether dispatch Linalg on tensors. I can make it still usable so you can control the behavior easier.

https://github.com/google/iree/blob/1a941fc4fd4398c83e168281e5cf657c663e8e40/iree/compiler/Translation/IREEVM.cpp#L132-L138

I'd like to delete duplicated and outdated e2e tests at least because it blocks mhlo.fft end-to-end testing. mhlo.fft only works in Linalg on tensors. What do you think?

@MaheshRavishankar
Copy link
Contributor

Lets keep this for now. If you want you can create a new column on Linalg on tensors project, and add this PR to that

The flag is not used. It now looks into the target backend to decide whether dispatch Linalg on tensors. I can make it still usable so you can control the behavior easier.

https://github.com/google/iree/blob/1a941fc4fd4398c83e168281e5cf657c663e8e40/iree/compiler/Translation/IREEVM.cpp#L132-L138

I'd like to delete duplicated and outdated e2e tests at least because it blocks mhlo.fft end-to-end testing. mhlo.fft only works in Linalg on tensors. What do you think?

I think deleting the tests is fine. We might still need the old flag to run the legacy path for perf comparison for maybe a week or so.

@hanhanW
Copy link
Contributor Author

hanhanW commented Apr 15, 2021

Lets keep this for now. If you want you can create a new column on Linalg on tensors project, and add this PR to that

The flag is not used. It now looks into the target backend to decide whether dispatch Linalg on tensors. I can make it still usable so you can control the behavior easier.
https://github.com/google/iree/blob/1a941fc4fd4398c83e168281e5cf657c663e8e40/iree/compiler/Translation/IREEVM.cpp#L132-L138

I'd like to delete duplicated and outdated e2e tests at least because it blocks mhlo.fft end-to-end testing. mhlo.fft only works in Linalg on tensors. What do you think?

I think deleting the tests is fine. We might still need the old flag to run the legacy path for perf comparison for maybe a week or so.

Okay, I will make the flag work and delete the duplicate tests in this PR instead.

@hanhanW hanhanW changed the title Delete iree-flow-dispatch-linalg-on-tensors flag. Delete duplicated tests in iree/test/e2e/ Apr 15, 2021
MaheshRavishankar
MaheshRavishankar requested changes Apr 16, 2021
View reviewed changes
iree/compiler/Dialect/Flow/Transforms/Passes.cpp Outdated
@@ -32,6 +32,12 @@
#include "mlir/Pass/PassRegistry.h"
#include "mlir/Transforms/Passes.h"

static llvm::cl::opt<bool> clEnableLinalgOnTensorsDispatch(
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be default true?

MaheshRavishankar
MaheshRavishankar requested changes Apr 16, 2021
View reviewed changes
iree/test/e2e/vulkan_specific/BUILD
@@ -74,9 +74,6 @@ iree_check_single_backend_test_suite(
"vectorized_conv.mlir",
],
compiler_flags = [
"-iree-flow-dispatch-linalg-on-tensors",
"-iree-codegen-spirv-experimental-linalg-on-tensors",
"-iree-spirv-enable-vectorization",
Copy link
Contributor

Choose a reason for hiding this comment

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

This flag probably needs to be kept (I am talking about just iree-spirv-enable-vectorization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we don't need it today. Vectorization is turned on by default in Linalg on tensors, see #5280

@hanhanW hanhanW merged commit 1bd938c into iree-org:main Apr 19, 2021
@hanhanW hanhanW deleted the del-e2e-tests branch April 19, 2021 16:30
@hanhanW hanhanW mentioned this pull request Apr 19, 2021
Closed
@GMNGeoffrey GMNGeoffrey mentioned this pull request Apr 19, 2021
Merged
@MaheshRavishankar MaheshRavishankar mentioned this pull request May 4, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaheshRavishankar MaheshRavishankar MaheshRavishankar approved these changes

@benvanik benvanik Awaiting requested review from benvanik

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hanhanW @MaheshRavishankar