[SYCL] Move buffer_impl and image_impl to the source directory

[romanovvlad]

The patch moves impl parts of buffer and image classes to the library to
give more freedom to modify their contents without breaking ABI.

To achieve this the patch:

1. Introduces buffer_plain and image_plain as a base classes for buffer
   and image respectively in order to introduce non-template proxy which
   can access impl parts.
2. Introduces initialization and finalization callbacks which are passed
   from headers to the library for the cases when source or destination
   heavily depends on the template parameters.
3. Adds several private methods which are accessed by "friend" classes.

[romanovvlad]

Expect at list windows symbols check test to fail.

[romanovvlad]

Please, let me know if it makes to split it to 2 PRs: one which introduces *_plain classes and another which does the move.

[romanovvlad]

Also removed a couple of "remove when ABI break is possible" related functions to avoid writing new in N-weeks dead code for them.

[aelovikov-intel]

Likely more for my education than anything else - why do we need both buffer_plain and buffer_impl ?

I think some outline of the design and interactions in the docs/comments would be of great help.

[romanovvlad]

Likely more for my education than anything else - why do we need both buffer_plain and buffer_impl ?

I think some outline of the design and interactions in the docs/comments would be of great help.

We have a separation for runtime classes: impl part and "wrapper/face" part. Wrapper part implements public SYCL API(including reference semantic) and usually contains just a pointer to the impl part only.
The impl part is unknown/opaque to the headers and real access happens in .cpp counterparts for such classes, following this approach we can freely change the details(e.g. add a new member/field) without breaking ABI.

This was not the case for the buffer and image. We haven't done this initially due to template nature of these classes - we cannot write an implementation of a template method of a template class, where template parameters can be custom user types, in the .cpp files. This patch tries to resolve this issue by introducing "plain"/non-templated base classes for them, so original templated classes can redirect all the calls to these base classes.

[aelovikov-intel]

I'm still not sure about that answer

// "->" represents a calls/uses relation.
template <...>
template<...>
                                +=----------------------------------+
RealClass::method<...>(...)  -> | RealClass_plain -> RealClassImpl  |
                                +-----------------------------------+

Why can't we call the whole box an impl and make it a single class? Maybe it's not exactly what "impl" means in implementations of other classes, but still. What is the purpose of separating plain from impl?

[romanovvlad]

I'm still not sure about that answer

// "->" represents a calls/uses relation.
template <...>
template<...>
                                +=----------------------------------+
RealClass::method<...>(...)  -> | RealClass_plain -> RealClassImpl  |
                                +-----------------------------------+

Why can't we call the whole box an impl and make it a single class? Maybe it's not exactly what "impl" means in implementations of other classes, but still. What is the purpose of separating plain from impl?

Probably I do not fully understand the idea

I'm still not sure about that answer

// "->" represents a calls/uses relation.
template <...>
template<...>
                                +=----------------------------------+
RealClass::method<...>(...)  -> | RealClass_plain -> RealClassImpl  |
                                +-----------------------------------+

Why can't we call the whole box an impl and make it a single class? Maybe it's not exactly what "impl" means in implementations of other classes, but still. What is the purpose of separating plain from impl?

Not sure I understand the proposal. The idea is that impl is not dereferenced in the headers.
Could you please tell how the arrow(->) from the picture above should look like in the code?

Consider we have this in public headers:

template<typename UserDefinedT> class PublicClass {
public:
  template<typename BB> void print(); // should call ImplT method to do the job
  std::shared_ptr<ImplT> impl;
};

How can we write an implementation of the PublicClass::print method without exposing impl details in the headers?

[aelovikov-intel]

So the answer to my question is this:

We need to separate interface and implementation of the buffer_impl because interface must be referenced in buffer.hpp due to buffer's template nature. We can't expose implementation there because that would expose ABI. As such two classes are required.

That actually suggests what an alternative approach might be:

// buffer_impl.hpp
struct buffer_impl_data; // Don't expose to the public sycl headers to hide ABI details

class buffer_impl {
public:
// declarations only for member functions;
private:
  std::unique_ptr<buffer_impl_data> data; // Don't expose ABI.
};

// buffer_impl.cpp
struct buffer_impl_data {
all the fields;
};

{
// "private" member functions for buffer_impl implemented as free functions here.
void helper(buffer_impl_data &data, ...);
}

// Definitions of member functions.
buffer_impl::buffer_impl(arg1, arg2)
  : data(std::make_unique<buffer_impl_data>(...) {
  data->field1 = arg1;
}

// buffer.hpp
template <class buffer> : public buffer_impl {
};

[cperkinsintel]

LGTM

[romanovvlad]

Just in case. Please do not merge so far.

[steffenlarsen]

Is the commented-out conjunction there because of the TODO or can they be removed?

[romanovvlad]

Yes.

[steffenlarsen]

LGTM!