Skip to content
This repository was archived by the owner on Mar 28, 2023. It is now read-only.

[SYCL] Test reduction for complex numbers.#1609

Merged
steffenlarsen merged 11 commits into
intel:intelfrom
uditagarwal97:complex_reduction
Mar 22, 2023
Merged

[SYCL] Test reduction for complex numbers.#1609
steffenlarsen merged 11 commits into
intel:intelfrom
uditagarwal97:complex_reduction

Conversation

@uditagarwal97

@uditagarwal97 uditagarwal97 commented Feb 22, 2023

Copy link
Copy Markdown

This PR adds test case for testing identityless reduction for complex numbers.

Corresponding intel/llvm PR: intel/llvm#8425

@uditagarwal97
uditagarwal97 requested a review from a team as a code owner February 22, 2023 16:17
@uditagarwal97 uditagarwal97 mentioned this pull request Feb 22, 2023
Merged
@uditagarwal97

uditagarwal97 commented Feb 22, 2023

Copy link
Copy Markdown
Author

/Verify with intel/llvm#8425

aelovikov-intel
aelovikov-intel reviewed Feb 22, 2023
View reviewed changes
Comment thread SYCL/Reduction/reduction_complex_nums.cpp Outdated
Comment thread SYCL/Reduction/reduction_complex_nums.cpp Outdated
@uditagarwal97

uditagarwal97 commented Feb 28, 2023

Copy link
Copy Markdown
Author

/verify with intel/llvm#8425

steffenlarsen pushed a commit to intel/llvm that referenced this pull request Mar 2, 2023
@uditagarwal97
[SYCL] Augment known_identity for std::complex and std::plus. (#8425)
2e73312 
This PR proposes to augment sycl::has_known_identity to return true for
std::complex and std::plus operator. This will have two benefits:

1. It enables support for complex numbers in sycl::reduction without the
user having to explicitly pass identity.
2. sycl::known_identity can now be used to simplify the implementation
of group algorithms (See PR #5394).

Also, this PR addresses the Github issue #5477.
Test Case PR: intel/llvm-test-suite#1609
@uditagarwal97

uditagarwal97 commented Mar 7, 2023

Copy link
Copy Markdown
Author

@steffenlarsen The PR is ready, please merge it. The failures in Jenkins CI are unrelated.

@steffenlarsen

steffenlarsen commented Mar 7, 2023

Copy link
Copy Markdown

@steffenlarsen The PR is ready, please merge it. The failures in Jenkins CI are unrelated.

Test seems to be failing on Windows. I believe you need #include <algorithm>.

steffenlarsen
steffenlarsen reviewed Mar 14, 2023
View reviewed changes
Comment thread SYCL/Reduction/reduction_complex_nums.cpp Outdated
@uditagarwal97

uditagarwal97 commented Mar 22, 2023

Copy link
Copy Markdown
Author

@steffenlarsen the test case is passing now for both Windows and Linux. There was a bug in the test case before:

The problem was in the test case, precisely in the following statement:std::generate(a.begin(), a.end(),                   [&n] { return std::complex<T>(n, ++n + 1); });

I was using this statement to initialize the buffer. However, the order of evaluation of the expression <n, ++n + 1> is undefined and that's why for different compilers on Linux and Windows, it was initializing the buffer differently.

steffenlarsen
steffenlarsen approved these changes Mar 22, 2023
View reviewed changes

@steffenlarsen steffenlarsen left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good find! Looks good. 🚀

@steffenlarsen
steffenlarsen merged commit 8d77241 into intel:intel Mar 22, 2023
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023
@uditagarwal97
[SYCL] Test reduction for complex numbers. (intel/llvm-test-suite#1609)
870c8c4 
This PR adds test case for testing identityless reduction for complex numbers.

Corresponding intel/llvm PR: intel#8425
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@aelovikov-intel aelovikov-intel aelovikov-intel approved these changes
@steffenlarsen steffenlarsen steffenlarsen approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@uditagarwal97 @steffenlarsen @aelovikov-intel