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

Unclear specification for __swizzle_vec__ type #504

Closed
gmlueck opened this issue Nov 28, 2023 · 3 comments · Fixed by #514
Closed

Unclear specification for __swizzle_vec__ type #504

gmlueck opened this issue Nov 28, 2023 · 3 comments · Fixed by #514

Comments

@gmlueck
Copy link
Contributor

gmlueck commented Nov 28, 2023

This question arose from a discussion with our DPC++ implementation team. The following statement in section 4.14.2.4 "Swizzled vec class" is unclear:

The __swizzled_vec__ class template should return __swizzled_vec__& for each operator inherited
from the vec class template interface which would return vec<DataT, NumElements>&.

Is this intended to apply to all the hidden friend functions of vec that are operators? For example, consider this operator:

friend vec operator+(const vec& lhs, const vec& rhs);

Is the spec saying that __swizzled_vec__ should define a hidden friend like this:

friend __swizzled_vec__ operator+(const __swizzled_vec__& lhs, const __swizzled_vec__& rhs);

What about other combinations of arguments, such as the ones below? Is the spec saying they should be defined also?

friend __swizzled_vec__ operator+(const __swizzled_vec__& lhs, const vec<DataT, NumElements>& rhs);
friend __swizzled_vec__ operator+(const vec<DataT, NumElements>& lhs, const __swizzled_vec__& rhs);

In addition, it's unclear what the effect of this operator should be. The __swizzled_vec__ class is supposed to represent the result of a swizzle operation before the operation is actually performed. Therefore, does this operator+ return a __swizzled_vec__ that also represents the addition without actually performing the addition?

This all seems like a lot of guesswork based on a terse statement in the spec. If this is really what we intend, it seems like the spec should state this more explicitly.

@gmlueck
Copy link
Contributor Author

gmlueck commented Nov 30, 2023

It was pointed out to me that the spec sentence I quote above:

The __swizzled_vec__ class template should return __swizzled_vec__& for each operator inherited
from the vec class template interface which would return vec<DataT, NumElements>&.

applies only to the operators that return a vec reference. Therefore, my example using operator+ is not correct. I think the intent of the spec is that the __swizzled_vec__ version of operator+ should return vec (not __swizzled_vec__). The spec is not clear about the types of the operands, but I presume we want to provide these overloads:

friend vec<DataT, NumElements> operator+(const __swizzled_vec__& lhs, const __swizzled_vec__& rhs);
friend vec<DataT, NumElements> operator+(const __swizzled_vec__& lhs, const vec<DataT, NumElements>& rhs);
friend vec<DataT, NumElements> operator+(const vec<DataT, NumElements>& lhs, const __swizzled_vec__& rhs);
friend vec<DataT, NumElements> operator+(const __swizzled_vec__& lhs, const DataT& rhs);

The sentence I quote does apply to the assignment operators like operator+=, though. I think it is saying that the implementation should provide these overloads:

friend __swizzled_vec__& operator+=(__swizzled_vec__& lhs, const __swizzled_vec__& rhs);
friend __swizzled_vec__& operator+=(__swizzled_vec__& lhs, const vec<DataT, NumElements>& rhs);
friend __swizzled_vec__& operator+=(__swizzled_vec__& lhs, const DataT& rhs);

I think it is still unclear what the effect of these operators should be. Does the returned __swizzled_vec__ represent the addition without actually performing the addition? (See my commentary in the original description of this issue.)

Although the spec does not say this, I presume we would also want this overload, which returns vec (not __swizzled_vec__):

friend vec<DataT, NumElements>& operator+=(vec<DataT, NumElements>& lhs, const __swizzled_vec__& rhs);

@gmlueck
Copy link
Contributor Author

gmlueck commented Dec 5, 2023

After chatting with @AerialMantis, I think I understand the intent better. Part of my confusion was with this statement in the spec:

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.

I interpreted that to mean that __swizzled_vec__::operator+= does not actually modify the vec and that the + operation is somehow represented by the __swizzled_vec__ that is returned. But, now I think this is a wrong interpretation.

I now think our intent with that sentence was to say that __swizzled_vec__ is a view of the underlying vec object, where the view shows the effect of the swizzle. Furthermore, I think our intent was that assignment operations like += do modify the underlying vec by adding to the elements that are selected by the swizzle.

My conversation with @AerialMantis also confirmed that the spec should clarify that the __swizzled_vec__ hidden friend functions should include the various overloads I show above.

I am preparing a PR to help clarify these things in the spec.

gmlueck added a commit to gmlueck/SYCL-Docs that referenced this issue Dec 5, 2023
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
@gmlueck
Copy link
Contributor Author

gmlueck commented Dec 5, 2023

See #514 for my attempt to clarify this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant