Skip to content

[SYCL] Fix sycl::span type deduction #4970

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

Merged
merged 3 commits into from
Nov 30, 2021
Merged

[SYCL] Fix sycl::span type deduction #4970

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6e95693
the deduction guide for array adjusted so it sends it to the correct …
cperkinsintel Nov 16, 2021
2aeacd0
updated comment per reviewer feedback
cperkinsintel Nov 17, 2021
86af71e
update comment. Should also rerun CI tests.
cperkinsintel Nov 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion sycl/include/CL/sycl/sycl_span.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,11 @@ as_writable_bytes(span<_Tp, _Extent> __s) noexcept
}

// Deduction guides
template <class _Tp, size_t _Sz> span(_Tp (&)[_Sz])->span<_Tp, _Sz>;

// array arg deduction guide. dynamic_extent arg used to select
// the correct template. The _Sz will be used for the __size of the span.
template <class _Tp, size_t _Sz>
span(_Tp (&)[_Sz]) -> span<_Tp, dynamic_extent>;
Copy link
Contributor

@keryell keryell Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I love comments! Thanks. Send more! :-)
But why does it work at the end and why we have _Sz ending in the type eventually?

Copy link
Contributor Author

@cperkinsintel cperkinsintel Nov 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@keryell , I think this previous discussion (the one that led to the comment in the first place) answers your question:
#4970 (comment)

But if it doesn't, perhaps you could rephrase it?

Regardless, it appears I should expand the comment even more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I see what the question is, and maybe I misspoke when answering @steffenlarsen earlier. The _Sz does not end up changing the extent (which is const) but ends up setting the __size of the span.

I guess a variation on these questions might be, "why is an array of known size end up constructing a span with dynamic_extent"? And while not the author of the code, I believe the answer to that is very simply that the dynamic_extent specialization tracks both data and size, while the other specialization does not. So the array constructor was placed there.

Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds to me like there's something wrong with the implementation. Correct me if I am wrong, but this way we won't have the "same" type deduced as for std::span, right? I.e.

int arr[] {1,2,3,4};
sycl::span SYCLSpan {arr}; // will have type sycl::span<int, sycl::dynamic_extent>
std::span STDSpan {arr};   // will have type std::span<int, 4>

Copy link
Contributor Author

@cperkinsintel cperkinsintel Nov 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, it looks like there is a problem in the implementation. I'll open up a ticket for it, rather than try to address it here. Being unable to instantiate spans at all from an array without a type specifier is a more immediate problem.

I went and compared GCC span, Clang libc++ span, and our SYCL span, and discovered that not only are we not handling fully specialized spans-from-arrays, but, surprisingly, partially specialized ones had dynamic extent in GCC and Clang stdlib, which I wasn't expecting.

int arr[] { 0, 1, 2, 3, 4, 5, 6, 7, 8 };
    
    SPAN<int,9> fully_specialized {arr};
    std::cout << typeid(fully_specialized).name() << std::endl;
    // GCC stdlib : St4spanIiLm9EE                           => std::span<int, 9ul>
    // clang stdlib : NSt3__14spanIiLm9EEE                   => std::__1::span<int, 9ul>
    // SYCL : won't compile. 
    
    SPAN<int> partially_specialized {arr};
    std::cout << typeid(partially_specialized).name() << std::endl;
    // GCC stdlib : St4spanIiLm18446744073709551615EE        => std::span<int, 18446744073709551615ul>
    // Clang stdlib: NSt3__14spanIiLm18446744073709551615EEE => std::__1::span<int, 18446744073709551615ul>
    // SYCL : N2cl4sycl4spanIiLm18446744073709551615EEE      => cl::sycl::span<int, 18446744073709551615ul>

    SPAN  unspecialized {arr};
    std::cout << typeid(unspecialized).name() << std::endl;
    // GCC stdlib : St4spanIiLm9EE                           => std::span<int, 9ul>
    // clang stdlib : NSt3__14spanIiLm9EEE                   => std::__1::span<int, 9ul>
    // SYCL : N2cl4sycl4spanIiLm18446744073709551615EEE      => cl::sycl::span<int, 18446744073709551615ul>

Copy link
Contributor

Choose a reason for hiding this comment

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

At least this shows it is good to add some comments and it can exhibit some bugs :-)


template <class _Tp, size_t _Sz> span(std::array<_Tp, _Sz> &)->span<_Tp, _Sz>;

Expand Down