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

Detect join member function with volatile-qualified arguments #4951

Merged
merged 9 commits into from
Apr 12, 2022

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Apr 8, 2022

Hotfix for #4931
For backward compatibility, make sure that user-defined reducer that only had a join member function that takes arguments with the volatile type qualifier are not rejected.

Fixes: #4952, #4941

@stanmoore1
Copy link
Contributor

Thanks!

@PhilMiller
Copy link
Contributor

One test failure in Jenkins, on OpenMPTarget:

4: [ RUN      ] openmptarget.reducers_struct
4: /var/jenkins/workspace/Kokkos/core/unit_test/TestReducers.hpp:325: Failure
4: Expected equality of these values:
4:   sum_scalar
4:     Which is: 32-byte object <00-E5 45-47 00-E5 45-47 00-20 D2-44 00-20 D2-44 00-E5 45-47 00-E5 45-47 00-E5 45-47 00-E5 45-47>
4:   reference_sum
4:     Which is: 32-byte object <00-E5 45-47 00-E5 45-47 00-E5 45-47 00-E5 45-47 00-E5 45-47 00-E5 45-47 00-E5 45-47 00-E5 45-47>
4: [  FAILED  ] openmptarget.reducers_struct (2 ms)

@PhilMiller
Copy link
Contributor

In kokkos-kernels develop, I get this test failure:

1: [  FAILED  ] 4 tests, listed below:
1: [  FAILED  ] cuda.iamax_double
1: [  FAILED  ] cuda.iamax_mv_double
1: [  FAILED  ] cuda.iamax_complex_double
1: [  FAILED  ] cuda.iamax_mv_complex_double
1: 
1:  4 FAILED TESTS
1/1 Test #1: blas_cuda ........................***Failed    8.26 sec

@PhilMiller
Copy link
Contributor

There may be misbehavior in the case where join() is overloaded for both volatile and not - the expression &F::join is problematic when there are two matching enable_if with different parameter types it could cast to that would select different overloads. It may still be failing back to operator+= in that case.

We may need separate has_join_*_function specializations, rather than separate overloads within the existing specializations

@PhilMiller
Copy link
Contributor

Removing the overload lets that pass.

@PhilMiller
Copy link
Contributor

dalg24#3 makes the overload a compilation error, rather than a silent runtime failure. Not sure it's a great path, but it does encourage code cleanup!

@PhilMiller
Copy link
Contributor

With the above addition to the PR, kokkos/kokkos-kernels#1382 is the minimal change to Kokkos Kernels to compile on OpenMP and CUDA and pass its tests on both

@PhilMiller
Copy link
Contributor

The OpenMPTarget failure may have something to do with making this call:

      Kokkos::parallel_reduce(Kokkos::RangePolicy<ExecSpace, ReducerTag>(0, N),
                              f_tag, reducer_scalar);

On a type array_reduce that has just operator+= and no join() member at all.

@PhilMiller
Copy link
Contributor

Your revision looks good.

Could you test that in the overloaded join case, the non qualified version gets selected? In my reading, that's what it looks like the code will do, but I'd like to see it ensured.

Should we include a runtime test that operator+= isn't being called in any of those cases? Maybe add an operator+= implementation that just fails the test to your compile test type definition, and make it runnable?

* Add test for backwards compatible join() detection

* Remove names of unused parameters to quiet warnings

* Mark as device/inline to be right-er and maybe suppress unused function warning
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 good to me.

@PhilMiller
Copy link
Contributor

I tested this with kokkos-kernels develop on OpenMP and CUDA, and it seems to be happy - tests all compile and pass.

I think we should merge this to unbreak KK CI testing, and follow-up with any further tests or refinements. So, marking this 'Ready'

@PhilMiller PhilMiller marked this pull request as ready for review April 12, 2022 03:16
@PhilMiller
Copy link
Contributor

May also fix #4941; testing now

@PhilMiller
Copy link
Contributor

retest this please

@crtrott
Copy link
Member

crtrott commented Apr 12, 2022

@dalg24 if this passes merge.

@dalg24 dalg24 merged commit 02ba049 into kokkos:develop Apr 12, 2022
Developer: Phil Miller automation moved this from In Review to Done, pending release Apr 12, 2022
@dalg24 dalg24 deleted the hotfix_volatile_qualifier_join branch April 12, 2022 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Developer: Phil Miller
Done in Release 3.7
5 participants