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

Bringup reduction silicon tests that were previously unsupported #806

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

jnie-TT
Copy link
Contributor

@jnie-TT jnie-TT commented Sep 24, 2024

We're using reduction and eltwise maximum ops in tt-xla FE but didn't have silicon tests for them, so we should bring them up.

@jnie-TT jnie-TT force-pushed the jnie/reduction_tests branch from 3d0a232 to e1d3c21 Compare September 24, 2024 15:46
@AleksKnezevic
Copy link
Contributor

@mrakitaTT, fyi

@mrakitaTT
Copy link
Contributor

@AleksKnezevic These tests will pass because keep_dim=True. The issue we have in #805 is that StableHLO reduce always kills the reducing dim, so I've used keep_dim=False in conversion, not knowing that Metal doesn't support that configuration yet. I am now trying some solution with ReduceOp with keep_dim=True and then ReshapeOp afterwards to manually kill the reducing dim, because otherwise the rest of the graph would be impacted.

@jnie-TT Thanks for adding more test cases! I've added test/ttmlir/Dialect/TTNN/simple_sum.mlir, test/ttmlir/Dialect/TTNN/simple_maximum.mlir and test/ttmlir/Dialect/TTNN/simple_max.mlir to cover these. It is weird that test/ttmlir/Dialect/TTNN/simple_max.mlir is not failing in CI since I've used keep_dim=False in that test O.o You can remove that one too, I'll add specific test for StableHLO conversion when I fix #805, but it would be good to know why it wasn't run in CI.

P.S. I've finally went through setting up a silicon machine because I'll need it for XLA, so going forward I'll manually test everything and not rely on CI.

@jnie-TT
Copy link
Contributor Author

jnie-TT commented Sep 24, 2024

@AleksKnezevic These tests will pass because keep_dim=True. The issue we have in #805 is that StableHLO reduce always kills the reducing dim, so I've used keep_dim=False in conversion, not knowing that Metal doesn't support that configuration yet. I am now trying some solution with ReduceOp with keep_dim=True and then ReshapeOp afterwards to manually kill the reducing dim, because otherwise the rest of the graph would be impacted.

@jnie-TT Thanks for adding more test cases! I've added test/ttmlir/Dialect/TTNN/simple_sum.mlir, test/ttmlir/Dialect/TTNN/simple_maximum.mlir and test/ttmlir/Dialect/TTNN/simple_max.mlir to cover these. It is weird that test/ttmlir/Dialect/TTNN/simple_max.mlir is not failing in CI since I've used keep_dim=False in that test O.o You can remove that one too, I'll add specific test for StableHLO conversion when I fix #805, but it would be good to know why it wasn't run in CI.

P.S. I've finally went through setting up a silicon machine because I'll need it for XLA, so going forward I'll manually test everything and not rely on CI.

@mrakitaTT please note that if you want a test to run on silicon, you need to add it under the Silicon folder. Adding it under the dialect folder will only test the compiler's ability to compile a specific mlir module, but will not generate a flatbuffer for execution on Silicon.

@mrakitaTT
Copy link
Contributor

@mrakitaTT please note that if you want a test to run on silicon, you need to add it under the Silicon folder. Adding it under the dialect folder will only test the compiler's ability to compile a specific mlir module, but will not generate a flatbuffer for execution on Silicon.

@jnie-TT Ohh, got it, thanks Jackson!

@jnie-TT
Copy link
Contributor Author

jnie-TT commented Sep 24, 2024

I'll merge this in now since we want any future CI to be running these.

@jnie-TT jnie-TT merged commit 93319b9 into main Sep 24, 2024
12 checks passed
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.

3 participants