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

Use custom sumInto function instead of Tpetra call #179

Closed
wants to merge 1 commit into from

Conversation

rcknaus
Copy link
Contributor

@rcknaus rcknaus commented Aug 28, 2017

  • Uses a sorted list of local ids to simplify avoid some work the Tpetra suminto performs

@rcknaus rcknaus requested a review from alanw0 August 28, 2017 16:46
Copy link
Contributor

@alanw0 alanw0 left a comment

Choose a reason for hiding this comment

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

Robert, I think the code itself looks ok. I have a couple of questions/comments:

  1. Since it has nothing to do with simd, maybe it should go into the master branch instead of the stk_simd_kernels branch.
  2. What is the performance benefit that it provides?
  3. We have a story in Jira, https://jira.exascaleproject.org/browse/ADSE07-141
    which is no longer needed if we take this approach. We should make a conscious decision that we are bypassing the tpetra/kokkos folks to some extent. Or, lobby them to include this code in the kokkos local-matrix that this Jira story was aimed at making use of.

@spdomin
Copy link
Contributor

spdomin commented Aug 28, 2017

Okay, is this ready to go or do we need to see a formal response from @rcknaus?

Also, yes, let's put this under master as I just accepted the SIMD update.

@rcknaus
Copy link
Contributor Author

rcknaus commented Aug 28, 2017

For performance, I saw about a 15% speedup on milestonRunConsolidated and about 10% speedup on hoHelium both run locally. From vtune, it looked like a 3x speedup for sum_into_row vs sumIntoValues on milestonRunConsolidated.

I'm not really sure what to do about #3.

@alanw0
Copy link
Contributor

alanw0 commented Aug 28, 2017

@rcknaus those sound like great performance improvements.
You don't need to do anything about #3, I'll close the Jira story.

@sayerhs sayerhs closed this Aug 29, 2017
@sayerhs
Copy link
Contributor

sayerhs commented Aug 29, 2017

@rcknaus Can you resubmit this against the master branch? Since the SIMD pull request was closed, I would like to avoid further commits on top of that.

@spdomin
Copy link
Contributor

spdomin commented Aug 29, 2017

We also need to have a performance sanity check, ideally, on a moving mesh simulation. My V27 mesh would be ideal. I want to make sure that setup costs do not change under this new formulation. Also, I was thinking that the change would only require sumInto to be modified, however, I see new data structures here and there. This is something I would like to understand better.

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.

4 participants