-
Notifications
You must be signed in to change notification settings - Fork 74k
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
Select in Generic Pattern Matching for FP8 GEMMs in XLA #61758
Select in Generic Pattern Matching for FP8 GEMMs in XLA #61758
Conversation
CC @reedwm. |
!Match(subgraph[i].first->operand(subgraph[i].second == 2 ? 1 : 2), | ||
m::Broadcast(m::ConstantScalar(0)))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test for the case where the other operand is nonzero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally don't have negative tests. Do we need one here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worthwhile to have one in this case, since otherwise we wouldn't catch bugs like if you hypothetically matched any ConstantScalar instead of a ConstantScalar of 1.
But for the sake of expediency in getting FP8 models working, I'll approve and merge now. Can you add a test in a follow up PR? If you are too busy I can also add one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me quickly add one here then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok i'll hold off on merging until you added the test
!Match(subgraph[i].first->operand(subgraph[i].second == 2 ? 1 : 2), | ||
m::Broadcast(m::ConstantScalar(0)))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worthwhile to have one in this case, since otherwise we wouldn't catch bugs like if you hypothetically matched any ConstantScalar instead of a ConstantScalar of 1.
But for the sake of expediency in getting FP8 models working, I'll approve and merge now. Can you add a test in a follow up PR? If you are too busy I can also add one.
6bffcec
into
tensorflow:master
Imported from GitHub PR tensorflow/tensorflow#61758 Adds Select to the set of ops supported by the generic pattern matching for FP8 GEMMs. Copybara import of the project: -- 4aa0659cac381691817b02cc5a33ac16ccc6c433 by Philipp Hack <phack@nvidia.com>: Support for select in generic pattern matching for FP8 GEMMs. -- 88ba2208aff790f5aa32f5c779ec32b0511b29e7 by Philipp Hack <phack@nvidia.com>: Support for select in generic pattern matching for FP8 GEMMs. -- 7b3b60cd73f10899e5980c14cd63616bdc5c5ef2 by Philipp Hack <phack@nvidia.com>: Support for select in generic pattern matching for FP8 GEMMs. Merging this change closes #61758 PiperOrigin-RevId: 561735390
Adds Select to the set of ops supported by the generic pattern matching for FP8 GEMMs.