-
Notifications
You must be signed in to change notification settings - Fork 14
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
End to End implementation of ttnn.prod op #1792
Conversation
1dd7293
to
54d4cbe
Compare
54d4cbe
to
f80c046
Compare
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.
It looks good. Please update and elaborate more on the new operation description in the dialect definitions.
f80c046
to
34a2156
Compare
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 have some concerns regarding restrictions imposed on much higher level of abstraction than the place where they belong to. We shouldn't make a StableHLO depend on TTNN, it's a very live project that changes every day, so even when some of those things get fixed we are still left with an unnecessary restrictions. On top of that we now have an infrastructure set in place to circumvent most (if not all) of those restrictions, so we should use it.
c2e90ee
to
46984d0
Compare
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.
This looks much better, but TTNN modelling is still an issue.
What's your opinion on this, to be quite honest this one
static Tensor invoke(
const Tensor& input,
bool all_dimensions = false,
int64_t dim = 0,
const bool keepdim = false,
const std::optional<MemoryConfig>& memory_config = std::nullopt);
looks terrible to me because of this part
bool all_dimensions = false,
int64_t dim = 0
but the second one lacks keepdim
(and is also a DPS).
test/ttmlir/Silicon/StableHLO/reduction/reduce_prod_op_full.mlir
Outdated
Show resolved
Hide resolved
c7bf20d
to
ca49a91
Compare
81b036b
to
59a1b7d
Compare
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.
Looks much better now, I left a few comments that should be addressed, but nothing critical.
59a1b7d
to
b184707
Compare
b184707
to
b633e89
Compare
closes #1790 #1791