Skip to content

[SYCL] span for SYCL2020 support #3569

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 15 commits into from
Apr 27, 2021
Merged

[SYCL] span for SYCL2020 support #3569

merged 15 commits into from
Apr 27, 2021

Conversation

cperkinsintel
Copy link
Contributor

SYCL2020 introduces span into the SYCL namespace, even when not compiling with C++20. I have adapted the Clang libcxx version of span here, divorcing it from its macros and private access to implementation details, but otherwise mostly intact.

@cperkinsintel
[SYCL] add span support
78b61e9 
SYCL2020 spec adds span to the sycl namespace, even when not compiling C++20.  I have adapted the Clang libcxx version of span here, divorcing it from its macros and private access to implementation details, but otherwise mostly intact.

Signed-off-by: Chris Perkins <[email protected]>
@cperkinsintel
overlooked test. restored.
1512d96 
Signed-off-by: Chris Perkins <[email protected]>
@cperkinsintel cperkinsintel requested a review from a team as a code owner April 17, 2021 02:59
@cperkinsintel cperkinsintel requested a review from rbegam April 17, 2021 02:59
@cperkinsintel cperkinsintel mentioned this pull request Apr 17, 2021
Closed
@cperkinsintel cperkinsintel marked this pull request as draft April 17, 2021 15:40
@cperkinsintel
Copy link
Contributor Author

I'm converting this to draft while I investigate the Win building issue. It builds ,runs and tests fine for me on Windows.
@vladimirlaz , the error messages I see here from the CI are similar (though not exactly the same) as the ones seen when we are not compiling with C++17. Yet, your uplift PR has been merged, and I can see the std=C++17 flag passed to cl.exe. Do you have any idea what might be the difference?

What version of MSVC are we using on the CI system? I wonder if that is the difference?

keryell
keryell reviewed Apr 18, 2021
View reviewed changes
Copy link
Contributor

@keryell keryell left a comment

Choose a reason for hiding this comment

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

That looks good

@vladimirlaz
Copy link
Contributor

I'm converting this to draft while I investigate the Win building issue. It builds ,runs and tests fine for me on Windows.
@vladimirlaz , the error messages I see here from the CI are similar (though not exactly the same) as the ones seen when we are not compiling with C++17. Yet, your uplift PR has been merged, and I can see the std=C++17 flag passed to cl.exe. Do you have any idea what might be the difference?

What version of MSVC are we using on the CI system? I wonder if that is the difference?

You can see that on configuration step:

-- The C compiler identification is MSVC 19.16.27044.0
-- The CXX compiler identification is MSVC 19.16.27044.0
-- The ASM compiler identification is MSVC
-- Found assembler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Professional/VC/Tools/MSVC/14.16.27023/bin/Hostx64/x64/cl.exe
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Professional/VC/Tools/MSVC/14.16.27023/bin/Hostx64/x64/cl.exe
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Professional/VC/Tools/MSVC/14.16.27023/bin/Hostx64/x64/cl.exe -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Professional/VC/Tools/MSVC/14.16.27023/bin/Hostx64/x64/cl.exe
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Professional/VC/Tools/MSVC/14.16.27023/bin/Hostx64/x64/cl.exe -- works

romanovvlad
romanovvlad reviewed Apr 20, 2021
View reviewed changes
keryell
keryell reviewed Apr 20, 2021
View reviewed changes
sycl/test/on-device/span/span.cpp
usm_span[0] += 100; // 101 modify first value using span affordance.

event E = Q.submit([&](handler &cgh) {
auto can_read_from_span_acc = SpanRead.get_access<access::mode::write>(cgh);
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind boggling this can_read with a mode::write :-)
I think the root cause is the buffer named SpanRead.
Just naming it buf or whatever would not cause all these semantic clashes...
Same for other accessors and buffers.
Use comments instead, if things need to be clarified.

@cperkinsintel cperkinsintel marked this pull request as ready for review April 22, 2021 19:58
@cperkinsintel
Copy link
Contributor Author

pinging codeowners

@romanovvlad romanovvlad merged commit 9356d53 into intel:sycl Apr 27, 2021
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Apr 28, 2021
Merge remote-tracking branch 'upstream/sycl' into l0_make_kernel_bundle
9c45cf7 
* upstream/sycl:
  [SYCL][Doc] Add group sorting algorithms extension specification (intel#3514)
  [Buildbot] Update Windows GPU driver to 27.20.100.9466 (intel#3594)
  [SYCL][NFC] Update tests for FPGA attributes (intel#3632)
  [CODEOWNERS] Add @kbobrovs back to few projects (intel#3638)
  [NFC] Update codeowners (intel#3619)
  [SYCL] Support 3-, 16-elements vectors in SG load/store (intel#3617)
  [SYCL-PTX] Fix libclc dependencies (intel#3624)
  [SYCL] Add sycl::span for SYCL2020 support (intel#3569)
  [NFC][SYCL] Avoid -Wreorder warning about order of initialization (intel#3620)
  [SYCL][DOC] Initial Draft of Extension for querying free device memory on Level Zero (intel#3468)
  [SYCL][PI][L0] Submit open command batch on event status query (intel#3612)
  [NFC] Fix the comment (intel#3613)
  Rename misleading attribute flag (intel#3610)
  [SYCL] Generate an opt report of kernel arguments.  (intel#3492)
  [SYCL] Support extra environment variables in LIT (intel#3598)
  [SYCL][Matrix] Make joint_matrix_mad return A*B+C's result instead of C=A*B+C (intel#3586)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keryell keryell keryell left review comments

@romanovvlad romanovvlad romanovvlad approved these changes

@rbegam rbegam Awaiting requested review from rbegam

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
@cperkinsintel @vladimirlaz @keryell @romanovvlad