8000 [SYCL][ESIMD] Fix ESIMD private global detection. by DenisBakhvalov · Pull Request #1909 · intel/llvm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[SYCL][ESIMD] Fix ESIMD private global detection. #1909

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
Merged
merged 1 commit into from
Jun 19, 2020
Merged

[SYCL][ESIMD] Fix ESIMD private global detection. #1909

merged 1 commit into from
Jun 19, 2020

Conversation

DenisBakhvalov
Copy link
Contributor

Signed-off-by: Denis Bakhvalov denis.bakhvalov@intel.com

@DenisBakhvalov DenisBakhvalov added the esimd Explicit SIMD feature label Jun 16, 2020
@DenisBakhvalov DenisBakhvalov requested a review from kbobrovs June 16, 2020 22:17
Fznamznon
Fznamznon reviewed Jun 17, 2020
View reviewed changes
Copy link
Contributor
@Fznamznon Fznamznon left a comment

Choose a reason for hiding this comment

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

I would expect some test and problem explanation.

@DenisBakhvalov
Copy link
Contributor Author

Actually, this was a bug introduced in this change.
This change is needed to fix the issue that is visible in #1854.
@kbobrovs can provide more details on the issue.

@DenisBakhvalov DenisBakhvalov requested a review from Fznamznon June 18, 2020 15:58
@DenisBakhvalov
Copy link
Contributor Author

This fix is required for #1854 to work.
Here is the error that this change fixes:

File /usr/lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/limits Line 206: SYCL explicit SIMD does not permit private global variable to have an initializer

Unfortunately, I can't write the test for it, since the SIMD runtime library itself is not merged (#1853 - #1856).

@Fznamznon
Copy link
Contributor

This fix is required for #1854 to work.
Here is the error that this change fixes:

File /usr/lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/limits Line 206: SYCL explicit SIMD does not permit private global variable to have an initializer

Unfortunately, I can't write the test for it, since the SIMD runtime library itself is not merged (#1853 - #1856).

Seems like diagnostic that you mentioned points to standard C++ headers. I think you don't need SIMD runtime library to write similar code in test and check that there is no diagnostics emitted.

@DenisBakhvalov DenisBakhvalov force-pushed the private/dbakhval-esimd-fix branch from e4f3d05 to a5bd5a2 Compare June 18, 2020 17:23
@DenisBakhvalov DenisBakhvalov requested a review from a team as a code owner June 18, 2020 17:23
@DenisBakhvalov
Copy link
Contributor Author

Ok. I added test.

@DenisBakhvalov DenisBakhvalov force-pushed the private/dbakhval-esimd-fix branch from a5bd5a2 to a05a42e Compare June 18, 2020 17:46
vladimirlaz
vladimirlaz previously approved these changes Jun 18, 2020
View reviewed changes
@kbobrovs
Copy link
Contributor

@DenisBakhvalov
The code fix looks OK, but I suggest to replace this test with a new test in clang/SemaSYCL which would not depend on sycl.hpp. This simple test should be enough to expose the problem AFAICT:

int x = 0;

@DenisBakhvalov
Copy link
Contributor Author

@kbobrovs the issue can be seen even on STL headers:
http://ci.llvm.intel.com:8010/#/builders/2/builds/3153
Anyway, the test is temporary. I will remove it after we merge SIMD library.

@kbobrovs
Copy link
Contributor

I don't mind, but it is FE code owners' call. As I recall usual practice is adding FE-only test not depending on other components when fixing FE code if possible.

kbobrovs
kbobrovs previously approved these changes Jun 18, 2020
View reviewed changes
Copy link
Contributor
@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

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

The Sema.h fix looks OK (brings it to the state we have internally, known to work fine).

@Fznamznon
Copy link
Contributor

I don't mind, but it is FE code owners' call. As I recall usual practice is adding FE-only test not depending on other components when fixing FE code if possible.

Each component needs it's own unit tests. Once you do change in FE, you need to add a FE-only test not depending on other components, including SYCL headers.

Fznamznon
Fznamznon reviewed Jun 18, 2020
View reviewed changes
sycl/test/basic_tests/esimd/check_compile.cpp Outdated
Comment on lines 7 to 8
// TODO: this a temporary test.
// Remove it after SIMD library is merged.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make it FE-only, non-temporary test.

Fznamznon
Fznamznon reviewed Jun 18, 2020
View reviewed changes
sycl/test/basic_tests/esimd/check_compile.cpp Outdated
@@ -0,0 +1,10 @@
// RUN: %clangxx -fsycl -fsycl-explicit-simd -fsycl-device-only -fsyntax-only -Xclang -verify %s
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't usually use clang driver in FE tests.

Suggested change
// RUN: %clangxx -fsycl -fsycl-explicit-simd -fsycl-device-only -fsyntax-only -Xclang -verify %s
// RUN: %clangxx -cc1 -fsycl -fsycl-is-device -fsycl-explicit-simd -fsyntax-only -verify %s

@DenisBakhvalov
[SYCL][ESIMD] Fix ESIMD private global detection.
fce8190 
Signed-off-by: Denis Bakhvalov <denis.bakhvalov@intel.com>
@DenisBakhvalov DenisBakhvalov dismissed stale reviews from kbobrovs and vladimirlaz via fce8190 June 18, 2020 18:55
@DenisBakhvalov DenisBakhvalov force-pushed the private/dbakhval-esimd-fix branch from a05a42e to fce8190 Compare June 18, 2020 18:55
@DenisBakhvalov DenisBakhvalov requested a review from Fznamznon June 18, 2020 18:55
@bader bader merged commit 616a396 into intel:sycl Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fznamznon Fznamznon Fznamznon approved these changes

@kbobrovs kbobrovs kbobrovs left review comments

@vladimirlaz vladimirlaz vladimirlaz left review comments

@elizabethandrews elizabethandrews Awaiting requested review from elizabethandrews

@premanandrao premanandrao Awaiting requested review from premanandrao

Assignees
No one assigned
Labels
esimd Explicit SIMD feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DenisBakhvalov @Fznamznon @kbobrovs @vladimirlaz @bader
0