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

[HIP] Fix issue w/ static initialization of function attributes #4242

Merged
merged 2 commits into from
Aug 17, 2021

Conversation

skyreflectedinmirrors
Copy link
Contributor

Fix issue w/ static initialization of function attributes on unspecialized type and add test.

This was a fun one to debug. Basically, on line 214 (https://github.com/kokkos/kokkos/compare/develop...arghdos:fix_launch_mechs?expand=1#diff-078df6e850082d467d9c91f0888ce3b7b5f5f0fcde51bc91a00845ec6dd9bfb0R214), we use a static initialization of the function attributes to "cache" them. However, this means that we will only get the function attributes for first kernel function pointer passed in. This means that on subsequent calls we will get (e.g.,) incorrect localSizeBytes, which results in an incorrect launch bound determination at runtime.

…lized type and add test

Change-Id: If09ba6647df0eda44bd00b55e29596179c9afe7b
Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

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.

Looks good, except the name of the alias that is bad. Please change it.

core/src/HIP/Kokkos_HIP_KernelLaunch.hpp Outdated Show resolved Hide resolved
@dalg24
Copy link
Member

dalg24 commented Aug 16, 2021

By the way I'd swear we ran into that exact same kind of bug in the past

@dalg24
Copy link
Member

dalg24 commented Aug 16, 2021

By the way I'd swear we ran into that exact same kind of bug in the past

Yup #3712

For the record this one was introduced in #3820

Change-Id: Iebc5067270aa677fd70d67e80eafa1f8b3104258
@dalg24 dalg24 merged commit d11068e into kokkos:develop Aug 17, 2021
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.

None yet

4 participants