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

data()==NULL after realloc with LayoutStride #351

Closed
ibaned opened this issue Jul 7, 2016 · 6 comments
Closed

data()==NULL after realloc with LayoutStride #351

ibaned opened this issue Jul 7, 2016 · 6 comments
Assignees
Labels
Bug Broken / incorrect code; it could be Kokkos' responsibility, or others’ (e.g., Trilinos) Enhancement Improve existing capability; will potentially require voting
Milestone

Comments

@ibaned
Copy link
Contributor

ibaned commented Jul 7, 2016

When running this example code:

#include <Kokkos_Core.hpp>

#include <cassert>

enum { SpatialDim = 3 };
enum { NNodes0 = 10 };
enum { NNodes1 = 20 };

static Kokkos::LayoutStride make_layout(int nnodes) {
  const int dimensions[] = {nnodes, SpatialDim};
  const int order[] = {1,0};
  const int rank = 2;
  return Kokkos::LayoutStride::order_dimensions(rank,order,dimensions);
}

int main(int argc, char** argv) {
  Kokkos::initialize(argc, argv);
#ifdef USE_STRIDE
  typedef Kokkos::View<double*[SpatialDim], Kokkos::LayoutStride> view_type;
#else
  typedef Kokkos::View<double*[SpatialDim]> view_type;
#endif
  view_type view;
#ifdef USE_STRIDE
  auto layout = make_layout(NNodes0);
  view = view_type("coords", layout);
#else
  view = view_type("coords", NNodes0);
#endif
#ifdef GOOD_REALLOC
  Kokkos::realloc(view, NNodes1, SpatialDim);
#else
  Kokkos::realloc(view, NNodes1);
#endif
  assert(view.size() == NNodes1 * SpatialDim);
  assert(view.dimension_0() == NNodes1);
  assert(view.dimension_1() == SpatialDim);
  assert(view.data() != nullptr);
  Kokkos::finalize();
}

With USE_STRIDE and without GOOD_REALLOC, the assertion view.data() != nullptr fails. The history here is my View used to have compile-time layout and realloc worked fine given only one dimension. After switching to LayoutStride I got segfaults due to this.

Either this shouldn't happen and there is a bug somewhere, or it is expected in which case I would prefer that some part of Kokkos threw a compile or runtime error on this example, so that its easier to track down the problem.

@hcedwar
Copy link
Contributor

hcedwar commented Jul 20, 2016

resize with a layout stride view should only accept layout object, not integer dimensions

@hcedwar hcedwar added the Bug Broken / incorrect code; it could be Kokkos' responsibility, or others’ (e.g., Trilinos) label Jul 20, 2016
@hcedwar hcedwar added this to the Backlog milestone Jul 20, 2016
@ibaned
Copy link
Contributor Author

ibaned commented Jul 20, 2016

realloc with a layout object argument fails to compile. what is the difference between resize and realloc ?

@hcedwar hcedwar added the Enhancement Improve existing capability; will potentially require voting label Jul 22, 2016
@hcedwar
Copy link
Contributor

hcedwar commented Jul 22, 2016

Resize and realloc were implemented to support the LayoutLeft and LayoutRight. Those functions were not previously intended to support LayoutStride. The fact that LayoutStride can be passed in at all is a bug. A new overload of resize and realloc that accepts a LayoutStride object is needed. Thus this issue is both a bug report and, I assume, a feature request.

@ibaned
Copy link
Contributor Author

ibaned commented Sep 16, 2016

Yes, thats correct, although the bug fix is all I'd need to consider it resolved, i.e. this usage ought to cause a compile or runtime error at the realloc call.

@crtrott crtrott self-assigned this Sep 19, 2016
@crtrott crtrott modified the milestones: Fall 2016, Backlog Sep 19, 2016
crtrott added a commit that referenced this issue Sep 19, 2016
This makes the resize and realloc functions which only take integers
work solely with LayoutLeft and LayoutRight. Added overloads which take
a view and a layout instance, which also works for LayoutStride.
Had to make Layout constructors explicit (i.e. prevent implicit calls
of Layout constructors). Also added missing default declarations of
constructors in LayoutStride.

Added tests for new resize and realloc and made the tests actually check
whether resize preserves old data.
@crtrott
Copy link
Member

crtrott commented Sep 19, 2016

OK here is how this works with LayoutStride:
instead of:

#else
  Kokkos::realloc(view, NNodes1);
#endif

do

#else
  Kokkos::realloc(view, make_layout(NNodes1));
#endif

The other version will now also compile time fail if the layout is not LayoutRight or LayoutLeft. Unit tests are put in place.

@ibaned
Copy link
Contributor Author

ibaned commented Sep 19, 2016

thanks very much !

@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) Enhancement Improve existing capability; will potentially require voting
Projects
None yet
Development

No branches or pull requests

3 participants