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.

Sign up for GitHub

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

Jump to bottom

[SYCL][DOC] Move sycl::complex's extension from proposed to experimental #9867

Closed
wants to merge 14 commits into from
Closed

[SYCL][DOC] Move sycl::complex's extension from proposed to experimental #9867

wants to merge 14 commits into from

Conversation

jle-quel
Copy link
Contributor

This PR proposed the modification of the sycl::complex's extension be moved from ./proposed to ./experimental, respecting the sycl/doc/extensions/README: The sycl::complex is implemented but may change in the future.

@jle-quel jle-quel requested a review from a team as a code owner June 14, 2023 07:45
steffenlarsen
steffenlarsen approved these changes Jun 14, 2023
View reviewed changes
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.

LGTM!

gmlueck
gmlueck reviewed Jun 15, 2023
View reviewed changes
Copy link
Contributor

@gmlueck gmlueck left a comment

Choose a reason for hiding this comment

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

Sorry, I think I missed this extension when it was first added to "proposed". I've added some comments below. I think most of these are just comments on the spec, so they should not affect the implementation.

sycl/doc/extensions/experimental/sycl_ext_oneapi_complex.asciidoc Show resolved Hide resolved
gmlueck
gmlueck reviewed Jun 21, 2023
View reviewed changes
Copy link
Contributor

@gmlueck gmlueck left a comment

Choose a reason for hiding this comment

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

Thanks, your previous commit did help clarify things. Since you added some new APIs, though, I have some new comments. :-)

sycl/doc/extensions/experimental/sycl_ext_oneapi_complex.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/experimental/sycl_ext_oneapi_complex.asciidoc Show resolved Hide resolved
sycl/doc/extensions/experimental/sycl_ext_oneapi_complex.asciidoc Show resolved Hide resolved
sycl/doc/extensions/experimental/sycl_ext_oneapi_complex.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/experimental/sycl_ext_oneapi_complex.asciidoc Show resolved Hide resolved
@bader bader requested a review from gmlueck June 22, 2023 22:15
@jle-quel
Copy link
Contributor Author

Hello @gmlueck @abagusetty 👋

After looking a bit more at the literal operators, there are some concerns regarding their introduction to this extension and its implementation.

Implementation-wise, the literal operators can't be used in device code due to the no support of long-double.
Overloading or modifying the declaration of the literal operators will not work.

Also, user-defined operators should be prefixed by _; this is not the case, and if a codebase already uses C++ standard literal operators (with using using namespace std::literals; this will conflict with the standard literal operators).

PR-wise, I'll remove all references of the literal operators from this PR, and will revert the deletation of the sycl/doc/extensions/proposed/sycl_ext_oneapi_complex.asciidoc so @abagusetty can modify the proposed and the implementation reflecting the concerns mentioned earlier.

@gmlueck
Copy link
Contributor

gmlueck commented Jul 18, 2023

The latest changes to the "experimental" extension look good.

I do not think we should simply restore the old "proposed" extension, though. When I do a "diff" between the "experimental" and "proposed" versions of the extension in this PR, there are many irrelevant differences.

My preference would be to do the following:

  • Remove the "Suffixes for complex number literals" section from the experimental extension (as you have done).
  • Do not restore the proposed extension in this PR.
  • Create a new PR which simply adds that section back to the experimental extension. You would probably open that PR after this one is merged.

This would allow discussion to continue on that section about literals until the issues are resolved. Once they are resolved, we can merge that second PR.

@jle-quel
Copy link
Contributor Author

Status: Waiting on revert PR for literal operators, which will then be merged into this PR and then this, will be ready to get merged

@jle-quel jle-quel mentioned this pull request Jul 21, 2023
Merged
steffenlarsen pushed a commit that referenced this pull request Jul 25, 2023
@jle-quel
Revert "[SYCL] Adds complex Literals support to sycl::experimental::c…
fc6d573 
…omplex" (#10521)

Reverts #9819

As mentioned in the PR #9867, this
introduction of literal operators creates some issues due to literal
operators receiving arguments as `long double` which are not supported
on device code (and only works on host side); and our worries that users
will try to use it and will not understand why it does not work.

Also, user-defined literal operators should be declared with a `_`
prefix to not clash with the standard literal operators when `using
namespace std::literals`.

Reverting this PR will facilitate the merge of the previously mentioned
PR and have a place where the issues from this introduction are listed.

Regardless of this PR, we'd still like to have support for literal
operators for complex, so if you (@abagusetty) could open up a new PR
for re-introducing the literal operator if a solution can be found also
to handle it in device code, that would be great!

@gmlueck @abagusetty
@jle-quel
Copy link
Contributor Author

Closing this branch for another PR, which will be created on TOP of sycl (containing the revert commit) to not add all the merge and delete commits from resolving the conflicts that happened during the lifetime of this PR.

@jle-quel jle-quel closed this Jul 25, 2023
@jle-quel jle-quel deleted the jle-quel/move-complex-extension-to-experimental branch July 25, 2023 14:27
@jle-quel jle-quel mentioned this pull request Jul 25, 2023
Merged
steffenlarsen pushed a commit that referenced this pull request Jul 27, 2023
@jle-quel @gmlueck
[SYCL][DOC] Update and Move sycl::complex's extension from proposed t…
ea6aea3 
…o experimental (#10553)

This PR proposed the modification of the sycl::complex's extension be
moved from ```./proposed``` to ```./experimental```, respecting the
```sycl/doc/extensions/README```: The ```sycl::complex``` is implemented
but may change in the future.

Reference PR: #9867

@abagusetty @gmlueck @steffenlarsen @AerialMantis

---------

Co-authored-by: Greg Lueck <gregory.m.lueck@intel.com>
mdtoguchi pushed a commit to mdtoguchi/llvm that referenced this pull request Oct 18, 2023
@jle-quel @mdtoguchi
Revert "[SYCL] Adds complex Literals support to sycl::experimental::c…
114298c 
…omplex" (intel#10521)

Reverts intel#9819

As mentioned in the PR intel#9867, this
introduction of literal operators creates some issues due to literal
operators receiving arguments as `long double` which are not supported
on device code (and only works on host side); and our worries that users
will try to use it and will not understand why it does not work.

Also, user-defined literal operators should be declared with a `_`
prefix to not clash with the standard literal operators when `using
namespace std::literals`.

Reverting this PR will facilitate the merge of the previously mentioned
PR and have a place where the issues from this introduction are listed.

Regardless of this PR, we'd still like to have support for literal
operators for complex, so if you (@abagusetty) could open up a new PR
for re-introducing the literal operator if a solution can be found also
to handle it in device code, that would be great!

@gmlueck @abagusetty
mdtoguchi pushed a commit to mdtoguchi/llvm that referenced this pull request Oct 18, 2023
@jle-quel @gmlueck @mdtoguchi
[SYCL][DOC] Update and Move sycl::complex's extension from proposed t…
949d854 
…o experimental (intel#10553)

This PR proposed the modification of the sycl::complex's extension be
moved from ```./proposed``` to ```./experimental```, respecting the
```sycl/doc/extensions/README```: The ```sycl::complex``` is implemented
but may change in the future.

Reference PR: intel#9867

@abagusetty @gmlueck @steffenlarsen @AerialMantis

---------

Co-authored-by: Greg Lueck <gregory.m.lueck@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abagusetty abagusetty abagusetty left review comments

@gmlueck gmlueck gmlueck approved these changes

@steffenlarsen steffenlarsen steffenlarsen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jle-quel @gmlueck @steffenlarsen @abagusetty