Host/Device accessors for mdspan

[fbusato]

Description

Provide specific host/device/managed accessors and mspan vocabulary types.

references:

• https://github.com/rapidsai/raft/blob/main/cpp/include/raft/core/host_device_accessor.hpp
• https://github.com/rapidsai/raft/blob/main/cpp/include/raft/core/host_mdspan.hpp

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[fbusato]

/ok to test

[fbusato]

/ok to test

[fbusato]

/ok to test

[mhoemmen]

Accessors are not required to be default constructible. Please see https://eel.is/c++draft/mdspan.accessor.reqmts for the actual requirements, and https://eel.is/c++draft/views.multidim#mdspan.mdspan.cons-1.4 for how mdspan constrains its default constructor on whether its accessor is default constructible.

[mhoemmen]

This is pretty easy to fix in C++20. I have to remind myself how the work-around works in C++ < 20.

Suggested change

	_CCCL_HIDE_FROM_ABI host_accessor() noexcept(__is_ctor_noexcept) = default;
	requires(std::is_default_constructible_v<_Accessor>) // needs C++20
	_CCCL_HIDE_FROM_ABI host_accessor() noexcept(__is_ctor_noexcept) = default;

[fbusato]

I tried to fix it. Without c++20, I cannot introduce a template parameter and use = default.
Also, I don't know what is the best way to use requires before c++20, without introducing a template parameter @miscco

[mhoemmen]

A key feature of any accessor is the set of permitted conversions, both implicit and explicit. mdspan depends on accessors' conversions in order for its own conversions to work. Implicit conversions make mdspan assignment work; explicit conversions make explicit construction of an mdspan of one type from an mdspan of another type work. Even default_accessor has a conversion to permit assigning an mdspan of nonconst to an mdspan of const.

An accessor's conversions are defined by its constructors and/or conversion operators. Not exposing those from the parent class means that mdspan of host_accessor<Accessor> won't be able to perform any of the conversions that mdspan of Accessor can perform.

A public using _Accessor::_Accessor; declaration would pull in all of the base class' constructors, but it wouldn't pull in any conversion operators.

Another issue is that _Accessor might have a virtual destructor. That would be weird, but it's legal in the Standard and nothing in this class prevents it.

[mhoemmen]

Section 5.8 of P2897R7 elaborates the design intent behind accessor conversions.

[mhoemmen]

A public using _Accessor::_Accessor; declaration would pull in all of the base class' constructors, but it wouldn't pull in any conversion operators.

For example, if Accessor A provides a conversion operator to B and a conversion operator to C, the only way to provide those in device_accessor<A> would be to use reflection to enumerate all conversion operators of A, and paste new ones into device_accessor<A> for device_accessor<B> and device_accessor<C>. While I'm not a C++ reflection expert, I don't think even P2996 reflection can do that, as it would require injecting member function definitions.

[fbusato]

I entirely missed that part. I added the constructors following the aligned_accessor model.

[fbusato]

/ok to test

[fbusato]

/ok to test

[mhoemmen]

Greetings and thanks for asking for feedback! : - ) I've added some comments.

[mhoemmen]

In a previous review, I mentioned the issue that this omits all the conversions to and from _Accessor that the _Accessor class defines. I just want to mention it again so I don't forget : - ) . Conversions are an important part of an accessor's interface, because they help define conversions for mdspan itself. For example, without default_accessor's converting constructor, you can't assign mdspan-of-nonconst to mdspan-of-const. Section 5.8 of P2897 elaborates this explanation.

Should we consider instead a CRTP base class approach to __*_accessor? That would let developers of custom accessors opt into the run-time pointer checking functionality.

[fbusato]

could be that you reviewed an older version? I previously added conversions from/to default_accessor for host_accessor and device_accessor.
Honestly, I'm was sure about managed_accessor because it looks unsafe...

[fbusato]

Should we consider instead a CRTP base class approach to __*_accessor? That would let developers of custom accessors opt into the run-time pointer checking functionality.

the idea is nice. I'm a bit concerned about the implications for users. Could host_accessor be roughly defined in the following way?

class host_accessor : public __host_accessor<cuda::std::default_accessor> { .. };

[mhoemmen]

@fbusato wrote:

could be that you reviewed an older version? I previously added conversions from/to default_accessor for host_accessor and device_accessor.

The issue is with transitive conversions for a possibly generic _Accessor.

For example, suppose that I define two accessors A and B. A is weaker (more type-erased) than B. A defines an implicit conversion from B, and B defines an explicit conversion from A. (This follows the idiom that explicit conversions assert preconditions, while implicit conversions have no preconditions.) One example of this case is A = default_accessor<float>, and B = aligned_accessor<float, 16>.

struct A {
  // ... other accessor stuff ...
  A(B) {}
};

struct B {
  // ... other accessor stuff ...
  explicit B(A) {}
};

The problem is, these conversions don't carry over to {host,device,managed}_accessor. For example, host_accessor<A> does not have an implicit conversion from host_accessor<B>, and host_accessor<B> does not have an explicit conversion from host_accessor<A>.

Here's a draft for how to fix this. I would need to prototype this a bit to make sure it works.

template<class _Accessor>
class host_accessor {
private:
  _Accessor acc_;

  // ... details ...
public:
  // ... other constructors ...

  // This case covers host_accessor<default_accessor<OtherElementType>> as well.
  template<class InputAccessor>
  // If _Accessor is constructible from InputAccessor, then
  // host_accessor<_Accessor> is constructible from
  // host_accessor<InputAccessor>.
  requires(is_constructible<_Accessor, const InputAccessor&>)
  constexpr
  // Conversion is explicit if there is no implicit conversion
  // from InputAccessor to _Accessor.
  explicit(! is_convertible_v<const InputAccessor&, _Accessor>)
  host_accessor(const host_accessor<InputAccessor>& input_acc)
    : acc_(input_acc)
  {}
};

[mhoemmen]

@fbusato I just sent you an offline message with a link to an example of how to do this correctly. wrapper (see below) is like host_accessor or device_accessor -- an accessor that wraps another accessor. The link explains the design and includes tests and use examples.

template<class Accessor>
requires(not is_wrapper_v<Accessor>)
class wrapper : public Accessor {
public:
  using offset_policy = typename Accessor::offset_policy;
  using element_type = typename Accessor::element_type;
  using reference = typename Accessor::reference;
  using data_handle_type = typename Accessor::data_handle_type;

  constexpr wrapper() noexcept 
    requires(std::is_default_constructible_v<Accessor>)
  = default;

  // "Wrapping constructor" -- takes the inner accessor.
  constexpr wrapper(const Accessor& input_acc)
    : Accessor(input_acc)
  {}

  template<class OtherAccessor>
  requires(
    std::is_constructible_v<Accessor, OtherAccessor>
  )
  constexpr
  explicit(
    not std::is_convertible_v<OtherAccessor, Accessor>
  )
  wrapper(const wrapper<OtherAccessor>& input_acc)
    : Accessor(input_acc)
  {}

  constexpr reference
  access(data_handle_type p, size_t k) const {
    return Accessor::access(p, k);
  }

  constexpr typename offset_policy::data_handle_type
  offset(data_handle_type p, size_t k) const {
    return Accessor::offset(p, k);
  }
};