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

const subview of const view with compile time dimensions on Cuda backend #310

Closed
dholladay00 opened this issue May 31, 2016 · 8 comments
Closed
Assignees
Labels
Bug Broken / incorrect code; it could be Kokkos' responsibility, or others’ (e.g., Trilinos)
Milestone

Comments

@dholladay00
Copy link

dholladay00 commented May 31, 2016

I have a const view View<const double*[N], LayoutRight, RandomAccess> a;
I try to assign a const view View<const double[N], LayoutRight, RandomAccess> b(subview(a,i,ALL())); which fails to compile.

If a is does not have const in it, then I do not receive a compile error (i.e. it compiles).

I have not tried auto, but that could work, would it yield the same thing (correct compile time dimensions, etc.)?

I have a MWE I can attach if need be.

This could be related to or an extension of issues #237, #257, and #258

@crtrott
Copy link
Member

crtrott commented May 31, 2016

This should work, and is probably related to the general extension of that assignment stuff. Let me reproduce it and then we can take it from there. Auto will probably not give you the correct compile time thingy. Also the RandomAccess complicates things a bit (but still this should work). Can you try without RandomAccess?

@crtrott crtrott added the Bug Broken / incorrect code; it could be Kokkos' responsibility, or others’ (e.g., Trilinos) label May 31, 2016
@dholladay00
Copy link
Author

dholladay00 commented Jun 1, 2016

Here is the MWE:

#include <Kokkos_Core.hpp>
#include <cstdio>

using Kokkos::parallel_for;
using Kokkos::MemoryTraits;
using Kokkos::RandomAccess;
using Kokkos::subview;
using Kokkos::ALL;
using Kokkos::LayoutRight;

typedef MemoryTraits< RandomAccess > RA;

template <class D, class ... P>
using View = Kokkos::View<D, P ... >;

int main (int argc, char* argv[]) {
  Kokkos::initialize(argc, argv);
  const int N = 10;
  // results in compile error
  View<const double*[4], LayoutRight, RA> A("A", N);

  // if use non-const instead, no compile error
  //View<double*[4], LayoutRight, RA> A("A", N);

  parallel_for (N, KOKKOS_LAMBDA (const int i)
  {
    View<const double[4], LayoutRight, RA> v(subview(A, i, ALL()));
  });

  Kokkos::finalize();
  return 0;
}

If you comment out the const A and un-comment the non-const A, then it compiles.

@ndellingwood ndellingwood added this to the Backlog milestone Jun 8, 2016
@dholladay00
Copy link
Author

@crtrott, would commit bd85283 also help fix this issue?

@crtrott
Copy link
Member

crtrott commented Jul 1, 2016

Not sure.

@crtrott
Copy link
Member

crtrott commented Jul 1, 2016

Ok looking at your reproducer: this is actually expected and correct behaviour. You can not allocate a View of const data. The reasoning is that you could never change the data. You can't even default initialize it, so why would you ever create it?

The way to handle RandomAccess trait is to create a non-const view, and then assign it to a const one.

That said it looks like the RandomAccess property prevents subviews. The following code works without RA but doesn't with RA:

#include <Kokkos_Core.hpp>
#include <cstdio>

using Kokkos::parallel_for;
using Kokkos::MemoryTraits;
using Kokkos::RandomAccess;
using Kokkos::subview;
using Kokkos::ALL;
using Kokkos::LayoutRight;

typedef MemoryTraits< RandomAccess > RA;

template <class D, class ... P>
using View = Kokkos::View<D, P ... >;

int main (int argc, char* argv[]) {
  Kokkos::initialize(argc, argv);
  const int N = 10;
  // results in compile error
  View<double*[4], LayoutRight> A("A", N);
  View<const double*[4], LayoutRight,RA> c_A = A;


  parallel_for (N, KOKKOS_LAMBDA (const int i)
  {
    View<const double[4], LayoutRight, RA> v(subview(c_A, i, ALL()));
  });

  Kokkos::finalize();
  return 0;
}

@dholladay00
Copy link
Author

Ah ok, so the best bet would probably be to have bigger view be non-const without any traits and assign const-ness and traits where the subview is constructed.

That makes sense, and I'm fairly certain I don't access the underlying data of the bigger view, it is only accessed from the subviews, so I don't think I should see a performance hit and it might decrease compile times?

I'll switch to that, and since we know that already works, I am ok with closing this issue.

@crtrott
Copy link
Member

crtrott commented Jul 1, 2016

Not this can't be closed, because the RA trait has to be assigned outside of a parallel for to have any effect in Cuda (there is a texture object we need to create). That the above thing from me doesn't compile is a true bug and needs to be fixed. We also need a unit test for this, since it clearly is not tested. I identified the place in the code where it goes wrong, we just need to find a solution to fix it.

@crtrott
Copy link
Member

crtrott commented Sep 11, 2016

The outstanding issue is actually the same as #385. I am close to have a fix.

@crtrott crtrott modified the milestones: Fall 2016, Backlog Sep 11, 2016
crtrott added a commit that referenced this issue Sep 11, 2016
This test subview generation with const, RandomAccess, and Atomic
trait. Total subview combinations tests was thus multiplied by 6.
crtrott added a commit that referenced this issue Sep 11, 2016
This fixes #385 and #310. To do so I had to change internal
interface for assigning data handles. This might affect
Stokhos/Sacado. Basically the subview assignment has to
give the handle and the offset as two separate things.
Before it was using raw pointers with offset added directly.
For handle types which need to keep offset as its own thing
(like Cuda Texture objects) this is not possible.
@crtrott crtrott closed this as completed Sep 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Broken / incorrect code; it could be Kokkos' responsibility, or others’ (e.g., Trilinos)
Projects
None yet
Development

No branches or pull requests

3 participants