gtensor icon indicating copy to clipboard operation
gtensor copied to clipboard

causing bugs: gtensor_span may not be contiguous

Open germasch opened this issue 4 years ago • 2 comments

I've seen this in two places now:

template <typename T, size_type N, typename S>
inline void gtensor_span<T, N, S>::fill(const value_type v)
{
  if (v == T(0)) {
    auto data = gt::backend::raw_pointer_cast(this->data());
    backend::standard::memset<S>(data, 0, sizeof(T) * this->size());
  } else {
    assign(*this, scalar(v));
  }
}

and also in the various copy overloads between device and spans. This makes an underlying assumption that the data kept in the span is contiguous, but that's only the case if the strides have been calculated from a shape, as gt::adapt does. Fundamentally, however, a gtensor_span can have arbitrary strides (e.g., we could and should do that for a nicer Fortran interface, but there are other reasons one might want to do this.)

germasch avatar Apr 26 '21 02:04 germasch

related to his: has_container_methods has essentially the same problem. If something is an actual container then yes, it'll have data() and size(), and the data will be contiguous, so can be fed to, e.g., blas. But for gtensor_span, that is not necessarily true. OTOH, It is possible that a view might be contiguous data, but it's probably lacking a data() method. The latter is more of a missing feature than a bug.

Also, in all cases, nothing is said about the order of the data, so even if it's contiguous, that may or may not be enough to do whatever one wants to do. But I think that's probably something that the user will just have to worry about.

germasch avatar Apr 26 '21 04:04 germasch

I think this has been partially addressed by is_f_contiguous (#237) and related changes, but may need further modifications.

bd4 avatar May 11 '23 13:05 bd4