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

[TF:MLIR] Improve parallelism of tf.AddN #42135

Merged
merged 2 commits into from
Aug 11, 2020

Conversation

WindQAQ
Copy link
Member

@WindQAQ WindQAQ commented Aug 7, 2020

No description provided.

@google-ml-butler google-ml-butler bot added the size:M CL Change Size: Medium label Aug 7, 2020
@gbaned gbaned self-assigned this Aug 7, 2020
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Aug 7, 2020
@gbaned gbaned requested a review from jpienaar August 7, 2020 18:09
@WindQAQ
Copy link
Member Author

WindQAQ commented Aug 7, 2020

/cc @smit-hinsu for visibility.

Copy link
Member

@jpienaar jpienaar left a 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

tensorflow/compiler/mlir/tensorflow/transforms/lower_tf.cc Outdated Show resolved Hide resolved
};

// Accumulate range `[begin, half)` and `[half, end)`,
// and add the results of two halves.
Copy link
Member

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)

Copy link
Member Author

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 |
Copy link
Contributor

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.

PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Aug 8, 2020
smit-hinsu
smit-hinsu previously approved these changes Aug 8, 2020
Copy link
Contributor

@smit-hinsu smit-hinsu left a 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)
Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice nit. Updated.

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Aug 8, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 8, 2020
PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Aug 8, 2020
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Aug 8, 2020
@WindQAQ WindQAQ requested a review from smit-hinsu August 8, 2020 00:11
Clarify range
Fix comment

Update index

Remove trivial word

Use only i to index
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Aug 8, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Aug 8, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 8, 2020
@google-ml-butler google-ml-butler bot added the kokoro:force-run Tests on submitted change label Aug 10, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 10, 2020
@tensorflow-copybara tensorflow-copybara merged commit aee9ca5 into tensorflow:master Aug 11, 2020
PR Queue automation moved this from Approved by Reviewer to Merged Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process size:M CL Change Size: Medium
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

7 participants