-
Notifications
You must be signed in to change notification settings - Fork 6
Allow MetaPhysicL to run on device using Kokkos #57
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
Conversation
There was too much branching introduced in the pointer implementation in order to avoid decrementing pointers past the array beginning This reverts commit 908952a.
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.
Not sure if I missed anything. In the future when part of a changeset is a huge diff but is a subset of something as simple as sed s/inline/METAPHYSICL_INLINE, put each such part in its own commit to avoid them being chaff obscuring other changes?
That would require planning ahead, not playing whack-a-mole with "calling a host function from a host-device function" warnings from nvcc |
… zero initialization before
|
Whack-a-mole work is why I finally gave in and learned to love git's oxymoronic ability to rewrite history; while I'm playing whack-a-mole my commit messages look like "Checkpoint - DO NOT MERGE", but when I'm done I interactive-rebase and (I sound more smug about this than I should - a proper history rewrite would be tested by a shell loop over the final |
|
Kokkos metaphysicl |
|
Current MetaPhysicL |
|
Am I reading that right? 15% slower? |
|
You must be looking at the flat number? Cumulatively the timing is 4.5% faster which I rendered in my brain as noise |
|
How are you linking Kokkos with MetaPhysicL btw? |
|
I think we need to be careful about Kokkos symbols. We don't want to expose it to every source file in MOOSE. |
|
In MOOSE, they are protected by |
|
I don't know how we'd make an upstream library config depend on a downstream one |
|
Just let MOOSE define |
|
I think we should be able to split headers to only expose host code to "host" MetaPhysicL types. Like I should remove the |
|
|
|
What's the ETA of this change at MOOSE? |
|
Maybe a month would be my guess if I stop lollygagging |
|
wow beautiful Kokkos recipe pass. Such wow |
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 want to take a crack at restoring the std:: option this weekend, see how hard it is and whether we should just merge without it and reintroduce it in a later PR; in the meantime a few questions
test/Makefile.am
Outdated
| BUILT_SOURCES = .license.stamp | ||
|
|
||
| .license.stamp: $(top_srcdir)/LICENSE | ||
| .license.stamp: |
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 lose the dependency 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.
There are warnings from autotools, something about a stamp not supporting a dependency
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'm not getting anything from the libMesh standard autotools (autoconf 2.72, automake 1.18.1, libtool 2.5.4, still the latest on ftp.gnu.org). Could you be more specific? And is nothing complaining about the same line in src/Makefile.am?
In the meantime I'm just going to restore this, as long as I'm pushing more commits anyway; that command does legitimately depend on that file, even though it's hardly a dependency we change often.
This is necessary when we're adding to that namespace in the backwards-compatibility build
If we're going to use a new ENABLE_FOO for backwards compatibility we need to be sure nobody's missing having it defined.
It's just plain easier to ensure that all definitions come after all the declarations they might need if the declarations are in a separate file.
I haven't gotten any warnings about this, and the one in src/Makefile.am seems to have been fine too.
There's some weird poorly-documented AC_CONFIG_HEADERS magic that lets us get away with omitting the path here, but it's unnecessarily confusing to rely on it.
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 I'm ready to merge this (once CI is happy, knock on wood; 2 recipes have passed so far), but I'll wait for confirmation that I haven't broken anything in your eyes.
|
And for the record, I've verified that https://github.com/manufactured-solutions/MASA still builds and passes tests against a |
|
And with a few changes, the MASA master branch now passes both with and without |
Co-authored-by: Alex Lindsay <[email protected]>
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 LGTM now
No description provided.