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

Disable multiple kernel instantiations when using HIP #4644

Merged
merged 1 commit into from
Jan 19, 2022

Conversation

Rombur
Copy link
Member

@Rombur Rombur commented Jan 5, 2022

Related to #4546

Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Did you consider other names for the option?
Should we spell out "kernel instantiation"?

Also please comment why you think the default should be OFF

@Rombur
Copy link
Member Author

Rombur commented Jan 5, 2022

Did you consider other names for the option?

I couldn't think of a good name. If you have a better one, I am ready to change.

Also please comment why you think the default should be OFF

Because that's why @crtrott told me to do.

@dalg24
Copy link
Member

dalg24 commented Jan 5, 2022

OpenMPTarget failure is clearly unrelated

4: /var/jenkins/workspace/Kokkos/core/unit_test/TestUniqueToken.hpp:151: Failure
4: Expected equality of these values:
4:   sum
4:     Which is: 9999990
4:   int64_t(N) * R
4:     Which is: 10000000
4: [  FAILED  ] openmptarget.unique_token_instance (261 ms)

@crtrott @rgayatri23 is that something worth investigating?

@rgayatri23
Copy link
Contributor

OpenMPTarget failure is clearly unrelated

4: /var/jenkins/workspace/Kokkos/core/unit_test/TestUniqueToken.hpp:151: Failure
4: Expected equality of these values:
4:   sum
4:     Which is: 9999990
4:   int64_t(N) * R
4:     Which is: 10000000
4: [  FAILED  ] openmptarget.unique_token_instance (261 ms)

@crtrott @rgayatri23 is that something worth investigating?

There is no implementation for the instance scope of UniqueToken yet. Only the global scope is implemented.
I thought that this test case was blocked for OpenMPTarget backend.

@@ -57,6 +57,7 @@ KOKKOS_ENABLE_OPTION(TUNING OFF "Whether to create bindings for tu
KOKKOS_ENABLE_OPTION(AGGRESSIVE_VECTORIZATION OFF "Whether to aggressively vectorize loops")
KOKKOS_ENABLE_OPTION(LAUNCH_COMPILER ON "Whether to potentially use the launch compiler")
KOKKOS_ENABLE_OPTION(COMPILE_AS_CMAKE_LANGUAGE OFF "Whether to use native cmake language support")
KOKKOS_ENABLE_OPTION(HIP_MULTIPLE_KERNEL_INSTANTIATIONS OFF "Whether multiple kernels are instantiated at compile time - improve performance but increase compile time")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
KOKKOS_ENABLE_OPTION(HIP_MULTIPLE_KERNEL_INSTANTIATIONS OFF "Whether multiple kernels are instantiated at compile time - improve performance but increase compile time")
KOKKOS_ENABLE_OPTION(HIP_MULTIPLE_KERNEL_INSTANTIATIONS OFF "Whether multiple versions for each kernel are instantiated at compile time - improve performance but increase compile time")

@crtrott crtrott added the Blocks Promotion Overview issue for release-blocking bugs label Jan 19, 2022
@dalg24
Copy link
Member

dalg24 commented Jan 19, 2022

Both HIP builds are passing. Failures are unrelated (issues with one of the testing machine)

@dalg24 dalg24 merged commit 0eee587 into kokkos:develop Jan 19, 2022
@Rombur Rombur deleted the less_inst branch September 19, 2022 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocks Promotion Overview issue for release-blocking bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants