Clarify __swizzled_vec__ class #514
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Prior to this commit, the `__swizzled_vec__` class was not precisely specified. We said that it had the same interface as `vec` except for a small set of differences that we enumerated. However, it became clear after discussing with the original authors that this wasn't a complete list of differences. This commit adds a complete list of the members and friend functions for the `__swizzled_vec__` type. In many cases, the descriptions simply defer to the `vec` specification, but there are also many cases where the semantics are special for `__swizzled_vec__`. The updated specification also describes `__swizzled_vec__` as a view over the original `vec` object, which I think is what we originally intended. I intentionally removed wording like "may not be bound to a l-value or escape the expression it was constructed in." I think the true restriction is that the lifetime of the `__swizzled_vec__` must not outlive the lifetime of the underlying `vec`, which is the case for any view. I also intentionally removed the statement: > Both kinds of swizzle member functions must not perform the swizzle > operation themselves, instead the swizzle operation must be performed > by the returned instance of `__swizzled_vec__` when used within an > expression, meaning if the returned `__swizzled_vec__` is never used > in an expression no swizzle operation is performed. This wording made is sound like the point of `__swizzled_vec__` was to delay the swizzle operation as a sort of optimization. However, our original intent was only to specify `__swizzled_vec__` as a view. This commit also uses our updated style to specify the `__swizzled_vec__` type, using a format that gives more horizontal space for the synopses and descriptions. Closes KhronosGroup#504
AerialMantis
reviewed
Dec 7, 2023
There was a problem hiding this comment.
This is a really good improvement, I like using the read/modified wording as a replacement to the lhs/rhs wording, and having the __swizzled_vec__ interface defined allows us to be more specific about some of the more nuanced behavior. I've made some suggestions. Thanks!
Improve the wording in the introduction of the section on `__swizzled_vec__` based on review comment feedback.
Addresses a code review comment. Some compiler implementations may not allow the lhs of a swizzled vector assignment to have repeated elements, so add a constraint to disallow this. It should be easy to implement this using normal SFINAE techniques.
Clarify the distinction between `__swizzled_vec__` and `__swizzled_vec__</*unspecified*/>` when used in the synopses.
I got feedback that the convention of putting quotes around "this" to indicate the C++ "this" object was confusing. I did this in cases where there was some ambiguity about which `__swizzled_vec__` we are referring to. Reword these sentences to find another way to clarify the ambiguity.
|
intel/llvm#11846 is not related to this PR. |
We discovered after some implemenation experience that the definitions proposed in the early commits of this PR don't support all of the desired use cases. A simple example is: ``` sycl::vec<int, 4> v; v.swizzle<1>()++; ``` This won't compile because the declaration of `operator++` takes a mutable reference to the swizzle object: ``` friend vec<DataT, NumElements> operator++(__swizzled_vec__& sv, int); ``` C++ language rules prohibit binding the temporary object returned by `v.swizzle<>()` to the mutable l-value reference parameter to `operator++`. To fix this, we changed all the swizzle operators to take a `const` reference to the swizzle object. This makes sense because the swizzle class is a view over the underlying `vec`, and member functions on a view class are normally `const`, even for functions that mutate the object that underlies the view. The logic here is that the view itself is not modified, only the object referenced by the view. This raises a new problem, though, because we don't want to allow code to change a `const vec` via a swizzle. For example, code like this should result in a diagnostic: ``` const sycl::vec<int, 4> v; v.swizzle<1>()++; ``` To fix this, we replace `__swizzled_vec__` with two classes: `__writeable_swizzle__` and `__const_swizzle__`. We use `__const_swizzle__` for swizzles of `const vec`, and this class provides only the non-mutating operators.
Now that we have two swizzle types (`__writeable_swizzle__` and `__const_swizzle__`), change the builtin functions to accept either of them.
We used to enclose all the member function descriptions in a table, so that the description and synopsis of each function was visually enclosed in a single table cell. This caused a problem with the PDF render, though, because some table rows were too big to fit on a single page, which causes an error when rendering the PDF. Fix this by eliminating the table. Instead, put a horizontal line between the descriptions of the functions to visually separate them.
Avoid line wrap in synopsis in PDF render.
|
I have removed the "draft" status from this PR. We have this working on a branch for DPC++ now, so we are confident that the API is implementable. |
Co-authored-by: Gordon Brown <gordon@codeplay.com>
Co-authored-by: Gordon Brown <gordon@codeplay.com>
TApplencourt
approved these changes
Jun 14, 2024
AerialMantis
approved these changes
Jun 25, 2024
Add a section documenting the destructors for the `__writeable_swizzle__` and `__const_swizzle__` classes. I debated whether we should require them to be trivial. I think they probably will be trivial in a typical implementation, but I wasn't certain enough to put this in the spec. For now, I just said that they have no visible effect. This allows an implementation to contain some internal state that is destroyed by the destructor, but I suspect most implementations won't require this.
This was referenced Jun 27, 2024
keryell
pushed a commit
that referenced
this pull request
Sep 10, 2024
Clarify __swizzled_vec__ class
gmlueck
pushed a commit
that referenced
this pull request
Nov 7, 2024
Clarify __swizzled_vec__ class (cherry picked from commit 695fa6d)
gmlueck
pushed a commit
that referenced
this pull request
Nov 7, 2024
Clarify __swizzled_vec__ class (cherry picked from commit 695fa6d)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this commit, the
__swizzled_vec__class was not precisely specified. We said that it had the same interface asvecexcept for a small set of differences that we enumerated. However, it became clear after discussing with the original authors that this wasn't a complete list of differences.We also discovered that the
__swizzled_vec__class should be split into two classes:__writeable_swizzle__and__const_swizzle__in order to reflect the different operations that are allowed on a swizzle of aconst vecvs. a swizzle of a modifiablevec. For example,v.swizzle<1>()++should not be allowed ifvis aconst vec.This commit adds a complete list of the members and friend functions for the
__writeable_swizzle__and__const_swizzle__types. These are collectively called the swizzled vector types. In many cases, the descriptions simply defer to thevecspecification, but there are also many cases where the semantics are special for the swizzled vector types.The updated specification also describes the swizzled vector types as a view over the original
vecobject, which I think is what we originally intended. I intentionally removed wording like "may not be bound to a l-value or escape the expression it was constructed in." I think the true restriction is that the lifetime of the swizzled vector type must not outlive the lifetime of the underlyingvec, which is the case for any view.I also intentionally removed the statement:
This wording made is sound like the point of
__swizzled_vec__was to delay the swizzle operation as a sort of optimization. However, our original intent was only to specify__swizzled_vec__as a view.This commit also uses our updated style to specify the swizzled vector types, using a format that gives more horizontal space for the synopses and descriptions.
Closes #504