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

Fix Kokkos_Sort when using integer and HIP #4570

Merged
merged 1 commit into from
Dec 14, 2021
Merged

Conversation

Rombur
Copy link
Member

@Rombur Rombur commented Dec 1, 2021

This is a fix required by vtkm. There is something weird happening with integer arithmetic and HIP. I don't understand why but some integer operations that should be fine (i.e. there is no overflow) are wrong. Casting to int64_t fixes the problem.

@dalg24
Copy link
Member

dalg24 commented Dec 2, 2021

Retest this please

@crtrott
Copy link
Member

crtrott commented Dec 2, 2021

5: [ RUN      ] sycl.mdrange_large_deep_copy
5: 
5: PI CUDA ERROR:
5: 	Value:           1
5: 	Name:            CUDA_ERROR_INVALID_VALUE
5: 	Description:     invalid argument
5: 	Function:        cuda_piEnqueueKernelLaunch
5: 	Source Location: /scratch/llvm/sycl/plugins/cuda/pi_cuda.cpp:2661
5: 
5: unknown file: Failure
5: C++ exception with description "Native API failed. Native API returns: -30 (CL_INVALID_VALUE) -30 (CL_INVALID_VALUE)" thrown in the test body.
5: [  FAILED  ] sycl.mdrange_large_deep_copy (370 ms)

any idea?

@masterleinad
Copy link
Contributor

5: [ RUN      ] sycl.mdrange_large_deep_copy
5: 
5: PI CUDA ERROR:
5: 	Value:           1
5: 	Name:            CUDA_ERROR_INVALID_VALUE
5: 	Description:     invalid argument
5: 	Function:        cuda_piEnqueueKernelLaunch
5: 	Source Location: /scratch/llvm/sycl/plugins/cuda/pi_cuda.cpp:2661
5: 
5: unknown file: Failure
5: C++ exception with description "Native API failed. Native API returns: -30 (CL_INVALID_VALUE) -30 (CL_INVALID_VALUE)" thrown in the test body.
5: [  FAILED  ] sycl.mdrange_large_deep_copy (370 ms)

any idea?

That's the error we tried to fix for CUDA in particular. I would not expect that this pull request changes anything. Did this actually work correctly for SYCL in the pull request that added this test?

@Rombur
Copy link
Member Author

Rombur commented Dec 2, 2021

I've rebased on develop. Let's see if that helps

@Rombur
Copy link
Member Author

Rombur commented Dec 2, 2021

Rebasing didn't help :/

@masterleinad
Copy link
Contributor

Rebasing didn't help :/

I think we just need to adapt the test for SYCL in some way...

@fnrizzi
Copy link
Contributor

fnrizzi commented Dec 8, 2021

@Rombur
When you say "some integer operations that should be fine (i.e. there is no overflow) are wrong" does this mean that even something "simple" like the following gives wrong result but what does wrong mean?
Is there something particular that pops up from looking at the result? or it seems totally random?

mul_(std::is_integral<typename KeyViewType::const_value_type>::value
                 ? 1.0 * max_bins__ / (int64_t(max) - int64_t(min))
                 : 1.0 * max_bins__ / (max - min)),

Does it give wrong result all the time or only for certain values of the parameters?
if the operation above is an example of a such failure maybe it would make sense to add a test for integer operations for HIP?

@Rombur
Copy link
Member Author

Rombur commented Dec 8, 2021

Hmm it looks like the problem is integer overflow. Not sure why I thought it wasn't.

@crtrott
Copy link
Member

crtrott commented Dec 10, 2021

@Rombur can you address francescos comment?

@Rombur
Copy link
Member Author

Rombur commented Dec 13, 2021

I already did

Copy link
Contributor

@fnrizzi fnrizzi left a comment

Choose a reason for hiding this comment

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

I guess we postpone adding a test tailored to check this?

@dalg24 dalg24 merged commit 7ff417b into kokkos:develop Dec 14, 2021
@Rombur Rombur deleted the sort 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants