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

Add Accessor, an interface for accessing buffers in kernels #1249

Closed

Conversation

bernhardmgruber
Copy link
Member

@bernhardmgruber bernhardmgruber commented Jan 28, 2021

This PR adds a new class template Accessor:

template<typename TMemoryHandle, typename TElem, typename TBufferIdx, std::size_t TDim, typename TAccessModes>
struct Accessor;

An accessor is an interface to access the data stored by a memory object.
Memory objects in alpaka are currently all objects fulfulling the view concept, which includes buffers.

An accessor is created via one of the following calls:

auto a = accessWith<Mode1, Mode2, ...>(memObj); // where ModeN is an access tag
auto a = access(memObj); // read/write
auto a = readAccess(memObj); // read
auto a = writeAccess(memObj); // write

Currently, alpaka defines the access modes via the tags ReadAccess, WriteAccess and ReadWriteAccess.
However, the user is free to add their own tags and specialize Accessor based on them.

The access functions construct an accessor by retrieving a memory handle from the memory object, which is stored in the accessor.
An accessor additionally has an element type TElem which is the type of the instances returned on access.
This type may be mutated depending on the access modes.
E.g. a read/write accessor will return a TElem& whereas a read accessor will just return TElem.
The TBufferIdx is the integral type used for indexing and index computations.
TDim is the integral dimension of the accessor.
TAccessModes is either one access tag or a std::tuple of access tags. In the latter case, the accessor behaves as if only the first access tag was specified.

Accessors with multiple access tags can be further constrained to a single access tag (contained in the list of existing tags) by again calling one of the access functions on them.

The alias BufferAccessor specifies the accessor type for the buffer object of a given accelerator and may simplify specifying a kernel's interface.

For data access, the accessor overloads operator() and operator[], which can be called with either TDim integrals or a corresponding Vec.

Releated issue: #38

example/bufferCopy/src/bufferCopy.cpp Outdated Show resolved Hide resolved
example/bufferCopy/src/bufferCopy.cpp Outdated Show resolved Hide resolved
example/bufferCopy/src/bufferCopy.cpp Outdated Show resolved Hide resolved
include/alpaka/mem/view/Accessor.hpp Outdated Show resolved Hide resolved
include/alpaka/mem/view/Accessor.hpp Outdated Show resolved Hide resolved
include/alpaka/mem/view/Accessor.hpp Outdated Show resolved Hide resolved
@bernhardmgruber
Copy link
Member Author

Here is a concept based way to declare if an Accessor at a kernel interface is read-only, or read-write: https://godbolt.org/z/7G8Gq5. It would look like this:

struct ReadWriteKernel {
    void operator()(const auto& acc, const Accessor auto& data){

    }
};

struct ReadOnlyKernel {
    void operator()(const auto& acc, const ConstAccessor auto& data){

    }
};

I think this is an abuse of concepts though.

@j-stephan
Copy link
Member

I have an item from my SYCL wish list:

  • Please include a (compile-time) way to identify the address space (host, global, shared, unified, PGAS, ...)! That is the main reason why I'm pushing hard for having alpaka accelerators because pointers lose so much information on the way.

I think this is an abuse of concepts though.

What about this?

template <typename TAcc>
struct ReadWriteKernel
{
    auto operator()(TAcc const& acc, Accessor<TAcc, int> data)
    {
    }
};

template <typename TAcc>
struct ReadOnlyKernel
{
    auto operator()(TAcc const& acc, Accessor<TAcc, int const> data)
    {
    }
};

This would also allow to specialize accessors for backends if required.

@j-stephan j-stephan linked an issue Feb 23, 2021 that may be closed by this pull request
@bernhardmgruber
Copy link
Member Author

bernhardmgruber commented Feb 23, 2021

We had a longer discussion today during the alpaka meeting on what further aspects should accessors should address:

  1. We should separate the access modes from the return type. That is, an alpaka accessor should have a list of supported access modes, which are specified as a type list template parameter.
  2. We should investigate whether the return type of the subscript operator on an accessor could be derived from the access mode. Ideally, this would result in a type trait yielding the return type given an access mode
  3. Access modes should be expressed via tag types, so they are extensible by a user, and not via an enum that cannot be extended.
  4. Since accessors can have multiple access modes, we should be able to create subaccessors from existing accessors, narrowing down the access modes. When an accessor with multiple access modes is accesses, one of the access modes must be chosen. Taking the first access mode could be a reasonable default. E.g. we want to create an Accessor<read> from an Accessor<write, read>. But if operator[] is invoked on the Accessor<write, read>, a write access will be performed and thus e.g. a mutable reference is returned.
  5. Accessors should support projection functions which are applied to the fetched element before it is returned to the user. It is sufficient to carry the type of this projection function and instantiate it temporarily at the time of access. No instance of the projection function needs to be stored inside the accessor.

@bernhardmgruber
Copy link
Member Author

bernhardmgruber commented Feb 23, 2021

I started implementing features 1, 2 and 3 and we need further clarification:

What is the return type of Accessor<..., T, ..., WriteOnly>::operator[]? If I return a T& the user could read from it. The only way to prevent this is by returning a proxy object which only allows assignment. Is this what we want? This would prevent e.g. calling functions with out parameters with buffers as arguments:

void f(float& out) {
    out = 42.0f;
}
struct Kernel {
    template<typename TAcc, typename Idx>
    auto operator()(TAcc const&, alpaka::Accessor<float*, float, Idx, 1, alpaka::WriteAccess> data) const {
        f(data[42]); // compile error
    }
};

Continuing with this thought, an accessor which could return a T& must be at least an Accessor<float*, float, Idx, 1, std::tuple<WriteAccess, ReadAccess>>. Again, is this what we want?

@bernhardmgruber
Copy link
Member Author

During the alpaka VC today we decided:

  • Drop the projection function: we could add an example on how to wrap an accessor with a projection.
  • Write-only accessors should be supported which return proxy types allowing only assignment.
  • We should have a separate ReadWrite access tag in addition to Read and Write access tags. A ReadWrite accessor cannot be constrained to a Read or a Write accessor. For this functionality, we need e.g. a accessor<ReadWrite, Read>, which can be constrained to a Read only or a ReadWrite accessor.

Copy link
Member

@psychocoderHPC psychocoderHPC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tiny annotations, in general, it looks good.

example/bufferCopy/src/bufferCopy.cpp Outdated Show resolved Hide resolved
example/bufferCopy/src/bufferCopy.cpp Outdated Show resolved Hide resolved
example/bufferCopy/src/bufferCopy.cpp Outdated Show resolved Hide resolved
include/alpaka/mem/view/Accessor.hpp Outdated Show resolved Hide resolved
include/alpaka/mem/view/Accessor.hpp Outdated Show resolved Hide resolved
include/alpaka/mem/view/Accessor.hpp Outdated Show resolved Hide resolved
@bernhardmgruber
Copy link
Member Author

I squashed all changesets so far into the implementation, update of copyright and documentation.

@bernhardmgruber
Copy link
Member Author

This PR is now only blocked by the CI still using clang-format 12.0.0. I proposed an upstream fix to the GitHub action here: DoozyX/clang-format-lint-action#36

@bernhardmgruber
Copy link
Member Author

To make this easier to review, I split off the feature from changing the tests and examples. Please review and merge this first: #1433.

@fwyzard
Copy link
Contributor

fwyzard commented Nov 4, 2021

From the PR description:

E.g. a read/write accessor will return a TElem& whereas a read accessor will just return TElem.

Why return a TElem and not a TElem const& ?
The extra copy may be expensive.

@fwyzard
Copy link
Contributor

fwyzard commented Nov 4, 2021

A further issue is that host side buffers can no longer be accessed without an accelerator. Since users could only turn on a CUDA accelerator for example, you can also not rely that e.g. a AccCpuSerial will always be available to access on the host.

This would be a blocker for adoption in CMS.

@bernhardmgruber
Copy link
Member Author

Why return a TElem and not a TElem const& ? The extra copy may be expensive.

Because when we started to design accessors, we foresaw their use also for textures and for hardware dependend load instructions. If an accessor is backed by an image, we cannot return a const reference, because there is no memory to point to. See a potential example here: #1253 (comment)
Similarly, if we would use an accessor that used e.g. __ldg, we could also not return a const reference.

The current status of this PR is to ship a minimum viable accessor for alpaka's views that has enough features for the SYCL backend living here #789.

A further issue is that host side buffers can no longer be accessed without an accelerator. Since users could only turn on a >> CUDA accelerator for example, you can also not rely that e.g. a AccCpuSerial will always be available to access on the host.

This would be a blocker for adoption in CMS.

The current design here does not have this problem. This problem would occur if we bind accessors to accelerators, which is something that comes up every now and then.

Be also aware that the split-off PR #1433 will merge accessors in an experimental namespace and not touch anything else. Alpaka will continue to support pointers as kernel arguments for the near future until we figure out how accessors can be used ergonomically. Or all SYCL vendors adopt SYCL 2020 and also support pointers in their API, in which case we could use a simpler design. We will see.

@bernhardmgruber
Copy link
Member Author

Since the feature is now merged by #1433, I would like to continue the discussion on the remainder of this PR, which is whether we adapt tests and examples, here: #1458. If there are any bugs and issues with the experimental accessor feature please open a separate issue.

Release 0.8 automation moved this from In progress to Done Nov 9, 2021
@sbastrakov
Copy link
Member

sbastrakov commented Nov 10, 2021

Isn't there somewhere (I guess Boost, but maybe also future C++) a trait that defines a best way to pass a constant value for a type - by value or by const reference

Edit: so it this is customizable for a combination of type + accelerator + memory level + whatever else we need, a user can define this copy hehavior and we can disable unsupported combinations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

abstract the memory access inside kernels
7 participants