Skip to content

[SYCL] span for SYCL2020 support - #3569

Merged
romanovvlad merged 15 commits into
intel:syclfrom
cperkinsintel:cperkins-span-support
Apr 27, 2021
Merged

[SYCL] span for SYCL2020 support#3569
romanovvlad merged 15 commits into
intel:syclfrom
cperkinsintel:cperkins-span-support

Conversation

@cperkinsintel

@cperkinsintel cperkinsintel commented Apr 17, 2021

Copy link
Copy Markdown
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

cperkinsintel commented Apr 17, 2021

Copy link
Copy Markdown
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

@keryell keryell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That looks good

Comment thread sycl/include/CL/sycl/sycl_span.hpp Outdated
Comment thread sycl/test/on-device/span/span.cpp Outdated
Comment thread sycl/test/on-device/span/span.cpp Outdated
Comment thread sycl/test/on-device/span/span.cpp Outdated
Comment thread sycl/test/on-device/span/span.cpp Outdated
Comment thread sycl/test/on-device/span/span.cpp Outdated
@vladimirlaz

vladimirlaz commented Apr 19, 2021

Copy link
Copy Markdown
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
Comment thread sycl/test/on-device/span/span.cpp
Comment thread sycl/test/on-device/span/span.cpp Outdated
Comment thread sycl/include/CL/sycl/sycl_span.hpp Outdated
Comment thread sycl/include/CL/sycl/sycl_span.hpp Outdated
keryell
keryell reviewed Apr 20, 2021
View reviewed changes
Comment thread sycl/include/CL/sycl/sycl_span.hpp Outdated
Comment thread 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);

@keryell keryell Apr 20, 2021

Copy link
Copy Markdown
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

cperkinsintel commented Apr 26, 2021

Copy link
Copy Markdown
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
Alexander Batashev
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)
jsji pushed a commit that referenced this pull request Feb 20, 2026
@svenvh @sys-ce-bb
Disable spirv-val in some tests (#3584)
6b47578 
Disable SPIR-V validation in some tests that do not pass with current
HEAD of SPIRV-Tools (cb38b234 ("spirv-opt: Add LegalizeMultidim...",
2026-02-11)) while the discussion of #3569 is ongoing.

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@8189551551d2bdb
KornevNikita pushed a commit that referenced this pull request Jun 11, 2026
@svenvh @KornevNikita
Disable spirv-val in some tests (#3584)
3f437c2 
Disable SPIR-V validation in some tests that do not pass with current
HEAD of SPIRV-Tools (cb38b234 ("spirv-opt: Add LegalizeMultidim...",
2026-02-11)) while the discussion of #3569 is ongoing.

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@8189551551d2bdb
pull Bot pushed a commit to VitalyAnkh/llvm that referenced this pull request Jul 13, 2026
@Qi-Ye-079 @jsji
Preventing generating FPRoundingMode for arithmetic constrained intri…
db8c687 
…nsics (intel#3771)

Fixes misuse of `FPRoundingMode` on some unsupported intrinsics which violates the OpenCL SPIR-V spec:

https://registry.khronos.org/OpenCL/specs/unified/html/OpenCL_Env.html#_rounding_modes_for_conversions

Such misuse can now be detected by `spirv-val`.

Mirrors the [same fix](llvm/llvm-project#182742) in the SPIR-V backend in LLVM.

Resolves intel#3569

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@148a0b9560a8dcc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rbegam rbegam Awaiting requested review from rbegam
2 more reviewers
@keryell keryell keryell left review comments
@romanovvlad romanovvlad romanovvlad 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.

4 participants

@cperkinsintel @vladimirlaz @keryell @romanovvlad