-
Notifications
You must be signed in to change notification settings - Fork 563
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
Fix CUDA and Albany builds from prior PR #1533 #4252
Fix CUDA and Albany builds from prior PR #1533 #4252
Conversation
Switch Kokkos::unordered_map to std::unordered_map as further work needs to be done resolving types first. Copy murmur hash code in directly to remove link error for Albany. To remove/refactor later for Kokkos development. Disabled one new test that needs modification to work with unordered map.
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
} | ||
} | ||
|
||
// TODO: Restore Kokkos unordered_map |
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.
You could have just used a HostSpace version of Kokkos::UnorderedMap
, but just using a standard library class should be fine too.
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.
@mhoemmen Thanks for reviewing this. Did not think of doing the host space which would have been easier. We are planning to do further development to use kokkos so eventually I'll be converting this back once I resolve some std::vector issues.
@@ -250,8 +261,10 @@ class Zoltan2_Directory { | |||
|
|||
// originally the nodes are stored in a hash but in the new Kokkos mode |
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.
Comment doesn't apply any more, but I want this fix to get in, so OK.
// TODO: Will be removed as Kokkos form develops | ||
#define ZOLTAN2_ROTL32(x,r) (uint32_t) \ | ||
(((uint32_t)(x) << (int8_t)(r)) | ((uint32_t)(x) >> (32 - (int8_t)(r)))) | ||
static inline void Zoltan2_MurmurHash3_x86_32 ( const void * key, int len, |
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.
The idiomatic way to have a static
(internal linkage) function in C++ is to put it in an anonymous namespace, like this:
namespace { // (anonymous)
void foo () {
whatever_code ();
}
} // namespace (anonymous)
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 will fix this.
FAIL_REGULAR_EXPRESSION "FAIL" | ||
) | ||
|
||
# Temporarily disabling this to resolve cuda build issues. |
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.
Shouldn't the above changes have fixed the build errors?
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 was just one of the issues - but was a new test with some complications so I just disabled. However if i use your suggestion of Host space for the unordered map I should be able to restore this as well.
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
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.
Approving for the sake of Albany, but does that disabled test actually work now?
@mhoemmen Thanks for the assistance on the review. I will make a follow up PR to implement your suggested changes. |
@MicheldeMessieres Thanks! Does the test actually build and pass with CUDA? |
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ mhoemmen ]! |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Using Repos:
Pull Request Author: MicheldeMessieres |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ mhoemmen ]! |
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
@mhoemmen To confirm tests were building and passing with CUDA which I tested on white. The PR fix is now merged. |
@MicheldeMessieres As long as the test is restored and passes with CUDA, I'm fine. It's not clear whether Kokkos::UnorderedMap would be faster for the sequential CPU case, so unless you plan on parallelizing the code, I wouldn't worry about it. |
@trilinos/zoltan2
Description
Repairs issues from PR #1533 for directory work.
Resolved CUDA tests and Albany link failure.
Motivation and Context
Restore CUDA builds and Albany build.
How Has This Been Tested?
On white:
source $TRILINOS_DIR/cmake/std/atdm/load-env.sh cuda-9.2-gnu-7.2.0-release-debug
$ cmake
-GNinja
-DTrilinos_CONFIGURE_OPTIONS_FILE:STRING=cmake/std/atdm/ATDMDevEnv.cmake
-DTrilinos_ENABLE_TESTS=ON -DTrilinos_ENABLE_Zoltan2=ON
$TRILINOS_DIR
$ bsub -x -Is -q rhel7F -n 16 ctest -j16
On clang parallel and serial.
Checklist