-
Notifications
You must be signed in to change notification settings - Fork 802
[ESIMD] Make all simd_view constructors non-public. #3557
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
Conversation
[ESIMD] Make all simd_view constructors non-public. This addresses long due ESIMD library review comments: // TODO @rolandschulz // {quote} // Why is this and the next constructor public ? Those should only be called // internally by e.g.select, correct ? // {/quote} // TODO @rolandschulz // {quote} // Is this intentional not a correct copy constructor (would need to be const // for that)? I believe we agreed that simd_view would have a deleted copy and // move constructor.Why are they suddenly back ? // {/quote}
9ed5b45 to
922ae96
Compare
| /// \ingroup sycl_esimd | ||
| template <typename BaseTy, typename RegionTy> class simd_view { | ||
| template <typename, int> friend class simd; | ||
| template <typename, typename> friend class simd_view; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed? Looks like you're trying to make a class be a friend of itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is needed. Otherwise you can't access private members - such as private constructors - from member functions.
Note that this is not a class, this is templated class. Each instantiation generates unrelated class which are not friends to each other, unless explicitly declared so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. This is to allow accessing private constructors from different template instantiations of a class. Makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just in case, I would like to confirm this with C++/FE experts, since it caught your and @pvchupin 's eyes.
@erichkeane, can you please take a look if this friend template class declaration below is a good way to allow
calling private constructor from select:
template <typename BaseTy, typename RegionTy> class simd_view {
template <typename, typename> friend class simd_view; // <=== all simd_view instantiations are friends
...
private:
simd_view(BaseTy &Base, RegionTy Region) : M_base(Base), M_region(Region) {}
simd_view(BaseTy &&Base, RegionTy Region) : M_base(Base), M_region(Region) {}
public:
template <int Size, int Stride, typename T = simd_view,
typename = sycl::detail::enable_if_t<T::is1D()>>
auto select(uint16_t Offset = 0) {
using TopRegionTy = region1d_t<element_type, Size, Stride>;
using NewRegionTy = std::pair<TopRegionTy, RegionTy>;
using RetTy = simd_view<BaseTy, NewRegionTy>; // <======= call private constructor
TopRegionTy TopReg(Offset);
return RetTy{this->M_base, std::make_pair(TopReg, M_region)};
}
...
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, thats a pretty typical way to do that. Note that the line you have labeled as 'call private constructor' isn't actually doing the call! That is a 'using' statement. It appears the 'return' line 2 lines lower is the one actually calling the private constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Yes, I should've put the comment below to return of course.
pvchupin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Red left hand side reworked looks ok to me.
| simd_view(simd_view &Other) = delete; | ||
| simd_view(simd_view &&Other) | ||
| : M_base(Other.M_base), M_region(Other.M_region) {} | ||
| public: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: access specifiers of deleted constructors is pretty meaningless here. This 'public' doesn't actually do anything as far as I remember.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just restoring the visibility for the following members (conversion, assignments,...) after injected private for the two constructors.
[ESIMD] Make all simd_view constructors non-public.
This addresses long due ESIMD library review comments below.
This may break backward compatibility, as some of the public APIs are removed. But ESIMD is experimental feature, this is allowed.
Real impact should be negligible, as
simd_viewobjects are most conveniently created withselectAPI.// TODO @rolandschulz
// {quote}
// Why is this and the next constructor public ? Those should only be called
// internally by e.g.select, correct ?
// {/quote}
// TODO @rolandschulz
// {quote}
// Is this intentional not a correct copy constructor (would need to be const
// for that)? I believe we agreed that simd_view would have a deleted copy and
// move constructor.Why are they suddenly back ?
// {/quote}
Signed-off-by: kbobrovs [email protected]