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

Use SFINAE to restrict possible View type conversions #2127

Closed
AndrewGaspar opened this issue May 7, 2019 · 3 comments
Closed

Use SFINAE to restrict possible View type conversions #2127

AndrewGaspar opened this issue May 7, 2019 · 3 comments
Assignees
Labels
Enhancement Improve existing capability; will potentially require voting

Comments

@AndrewGaspar
Copy link
Contributor

AndrewGaspar commented May 7, 2019

Currently the Kokkos::View constructors are pretty permissive about what they accept, meaning View conversions cannot be disambiguated when there are two possible View type targets. For example, the compiler will not compile this code even if the default memory space is HostSpace since it looks like the argument conversion is “viable” for both routines:

void foo(View<double **, HostSpace>);
void foo(View<int **, HostSpace>);

void bar() {
    View<double **> v;
    foo(v);
}

This code results in this error:

/Users/agaspar/Code/playground/views.cpp:10:5: error: call to 'foo' is ambiguous
    foo(v);
    ^~~
/Users/agaspar/Code/playground/views.cpp:5:6: note: candidate function
void foo(View<double **, HostSpace>);
     ^
/Users/agaspar/Code/playground/views.cpp:6:6: note: candidate function
void foo(View<int **, HostSpace>);
     ^
1 error generated.

The Kokkos View conversion constructors should use SFINAE so the constructor is only valid for possible conversions.

@crtrott crtrott added the Enhancement Improve existing capability; will potentially require voting label May 7, 2019
@crtrott crtrott self-assigned this May 7, 2019
@crtrott
Copy link
Member

crtrott commented May 9, 2019

There are some tricky issues about long term backwards compatibility if I were to use our current "is_assignable" check as the enabling capability. In the past we figured out new corner cases which should be assignable and enabled that. With overload resolution now coming into play, this would potentially break code which relies on specific overload resolution cases.

In order to limit the potential for problems in that direction, I am going to only use a is_assignable_data_type thing. Which means that you can distinguish by value_type, by rank and by different compile time dimensions. I.e. this now compiles:

#include<Kokkos_Core.hpp>

// Overload based on value_type and rank
void foo(Kokkos::View<const double**,Kokkos::HostSpace> a) {
  printf("const double**\n");
}
void foo(Kokkos::View<const int**,Kokkos::HostSpace> a) {
  printf("const int**\n");
}
void foo(Kokkos::View<const double***,Kokkos::HostSpace> a) {
  printf("const double***\n");
}

// Overload based on compile time dimensions
void bar(Kokkos::View<double*[3],Kokkos::HostSpace> a) {
  printf("const double*[3]\n");
}
void bar(Kokkos::View<double*[4],Kokkos::HostSpace> a) {
  printf("const double*[4]\n");
}
// This would be ambigous
//void bar(Kokkos::View<double**,Kokkos::HostSpace> a) {
//  printf("const double*[4]\n");
//}
//void bar(Kokkos::View<double*[3],Kokkos::LayoutLeft,Kokkos::HostSpace> a) {
//  printf("const double*[3]\n");
//}


int main(int argc, char* argv[]) {
  Kokkos::initialize(argc,argv);
  {
     Kokkos::View<double**,Kokkos::LayoutRight,Kokkos::HostSpace> a("A",10,3);
     foo(a);
     foo(Kokkos::View<const double**,Kokkos::LayoutRight,Kokkos::HostSpace>(a));
     Kokkos::View<double***,Kokkos::LayoutRight,Kokkos::HostSpace> b("B",10,3,4);
     foo(b);
     Kokkos::View<double*[3],Kokkos::LayoutRight,Kokkos::HostSpace> c(a);
     bar(c);
     // This would be ambigous
     //bar(a);
  }
  Kokkos::finalize();
}

Note functions which are overloaded based on compile time size of a dimension are still ambiguous with respect to runtime dimensions!

@AndrewGaspar
Copy link
Contributor Author

AndrewGaspar commented May 9, 2019

That sounds good to me. Interpreting this:

Which means that you can distinguish by value_type, by rank and by different compile time dimensions.

Makes me assume that, essentially, I couldn't have a version of foo that takes HostSpace and a foo that takes CudaSpace and expect it to disambiguate. Which is fine, but I just want to make sure my interpretation is correct.

@crtrott
Copy link
Member

crtrott commented May 9, 2019

Yes that is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improve existing capability; will potentially require voting
Projects
None yet
Development

No branches or pull requests

3 participants