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

[SYCL][DOC] Update marray specification #12786

Open
wants to merge 6 commits into
base: sycl
Choose a base branch
from

Conversation

jle-quel
Copy link
Contributor

@jle-quel jle-quel commented Feb 21, 2024

Description

In response to feedback from @steffenlarsen on the specification implementation (#8647), several adjustments were made:

  • Corrected missing operators identified in the specification (See 3rd commit: 3b35704)
  • Removed unnecessary operators initially added (See 3rd commit: 3b35704)
  • Applied formatting and reordering for improved readability (See 1st and 4th commit: ff1f4e3, 7147da4)

Question

Additionally, I question the need to explicitly delete operators, considering they are not automatically generated nor inherited from the generic marray. I propose an approach to only include the necessary operators for implementation (See 2nd commit: 300a22e).

If agreed, the part that stipulates to delete the operators and the deleted operators in the inline code would be removed. (Explaining why this PR is a Draft)

If not, this raises a question for future implementers: Is it necessary to explicitly delete all operators, or can the declarations be omitted?

Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Additionally, I question the need to explicitly delete operators, considering they are not automatically generated nor inherited from the generic marray. I propose an approach to only include the necessary operators for implementation (See 2nd commit: 300a22e).

You are right, they are not required to be explicitly defined, but I would argue that the benefit of defining them is that we express explicit intent. That is, by specifying that these operators are deleted, the user can easily see that we intentionally disallow them and it tells us that whoever wrote the extension thought about the operators and concluded that they should not be there and should not be added in the future.

@jle-quel jle-quel marked this pull request as ready for review May 6, 2024 12:42
@jle-quel jle-quel requested a review from a team as a code owner May 6, 2024 12:42
@gmlueck
Copy link
Contributor

gmlueck commented May 30, 2024

I'm not opposed to this PR, but I think the entire section "Marray Complex Class Specialization" is not really necessary. The marray operators are available for any DataT that is a C++ Numeric Type. Since the complex type is a Numeric Type, they are automatically supported by marray.

I agree that we need some documentation about which operators are available. There is a proposal in the Khronos issue DB that the SYCL specification should have general wording saying that each marray operator is only available if the underlying DataT supports that operator (e.g. operator+ is only supported on marray<DataT> if operator+ is also supported on DataT). See Khronos issue 559, which requires access to the Khronos issue DB.

@jle-quel
Copy link
Contributor Author

jle-quel commented Nov 4, 2024

I understand your comment @gmlueck but do you believe that we should first fix the specification for what it already is?

This PR is not the one that introduces the Marray Complex Class Specialization but is opened
to fix and update some of the comments that have been previously made.

We could right after, open another PR that continues the conversation you started? (also asking your opinion @steffenlarsen )

PS: With the possible removal of the Marray Complex Class Specialization would that mean as well that the marray specialization implementation is not relevant anymore?

@gmlueck
Copy link
Contributor

gmlueck commented Nov 4, 2024

PS: With the possible removal of the Marray Complex Class Specialization would that mean as well that the marray specialization implementation is not relevant anymore?

This is a good question. It seems like the base definition of marray should work with any numerical type, which would include complex. Why do we need to implement any specialization? One reason might be performance. Are we implementing the specialization in order to optimize the complex case somehow? It seems like we should not need to specialize in order to have a correct implementation.

@jle-quel
Copy link
Contributor Author

jle-quel commented Nov 5, 2024

PS: With the possible removal of the Marray Complex Class Specialization would that mean as well that the marray specialization implementation is not relevant anymore?

This is a good question. It seems like the base definition of marray should work with any numerical type, which would include complex. Why do we need to implement any specialization? One reason might be performance. Are we implementing the specialization in order to optimize the complex case somehow? It seems like we should not need to specialize in order to have a correct implementation.

As you said, the base definition of marray supports any numerical type, thus including complex.
I ran the tests from the marray specialization PR without the specialization and all tests passes.

The specialization when first introduced, was here to add free functions that mimics the complex class.
In other words getter/setter API for real and imag as presented in the first reference to the specialization.

We already had a discussion on these free functions, and we came to the conclusion that this should not be added to the marray specialization (see #8852).

The specialization is now just here to delete the inapplicable operators not supported by complex but does not add any optimization in the specialization.

With your latest comment and looking at it with fresh eyes, makes me question the relevance of the specialization.

Any counter arguments on this @steffenlarsen ?

PS: The operators needed to be hidden friends, but the base marray definition now defines the operators this way.

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 this pull request may close these issues.

3 participants