-
Notifications
You must be signed in to change notification settings - Fork 74.2k
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
[TF:MLIR] Improve parallelism of tf.AddN #42135
Conversation
/cc @smit-hinsu for visibility. |
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.
Nice, thanks, one suggestion
}; | ||
|
||
// Accumulate range `[begin, half)` and `[half, end)`, | ||
// and add the results of two halves. |
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.
Why two halves vs a tree of additions? (meaning this is an improvement but if the goal is additional parallelism one could go further)
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.
Sounds good! I change it to tree-based reduction. Please check it out 😃
// | | | | | | ||
// ------- ------- | | ||
// | | | | ||
// 1 5 | |
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.
nit: 1 is used twice.
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.
Thanks for the contributions!
I am curious to learn about your application of this and how much this change helped you.
int64_t j = 0; | ||
for (int64_t i = 0; i < n; i += 2, ++j) { | ||
// Add two adjacent operands if applicable. | ||
operands[j] = (i + 1 < n) |
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.
nit: we can use operands[i/2] and n = (n+1)/2.
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.
Nice nit. Updated.
Clarify range
Fix comment Update index Remove trivial word Use only i to index
No description provided.