[SYCL] Add size to detail::string_view #18661

Alexandr-Konovalov · 2025-05-26T08:53:51Z

This allows us to pass size of kernel name that is known in compile time, not compute the size in run time.

Signed-off-by: Alexandr Konovalov <[email protected]>

frasercrmck · 2025-05-26T14:26:43Z

FYI I see the same sort of Arc A-Series failures, tracked in #18668.

Signed-off-by: Alexandr Konovalov <[email protected]>

jbrodman · 2025-05-27T17:25:25Z

So the basic idea here is that we compute the string length once at the start then can reuse the length every time we create a new view of it?

Alexandr-Konovalov · 2025-05-28T07:39:41Z

So the basic idea here is that we compute the string length once at the start then can reuse the length every time we create a new view of it?

Actually, KernelNameStr is constexpr and I see no string length computation at run time.

Signed-off-by: Alexandr Konovalov <[email protected]>

steffenlarsen

Overall LGTM! One smallish suggestion.

steffenlarsen · 2025-06-02T09:34:45Z

sycl/include/sycl/detail/kernel_name_str_t.hpp

 using KernelNameStrRefT = std::string_view;
 using ABINeutralKernelNameStrT = detail::string_view;
+
+inline KernelNameStrT toKernelNameStrT(ABINeutralKernelNameStrT str) {


Do the str argument constness need to differ between the definitions? I would prefer we keep them the same, even if it means removing const in the variant below. If it gets removed with breaking changes, I would argue it's better to have it the same and avoid the potential hidden faulty const use-cases we could have.

Sorry, probably I do not understand. Are you for passing detail::string_view by const reference, like this?

inline KernelNameStrT toKernelNameStrT(const ABINeutralKernelNameStrT &str) { #ifdef __INTEL_PREVIEW_BREAKING_CHANGES return std::string_view(str); #else return str.data(); #endif }

If that is an option, that would be good! My point is just to avoid differing constness of the reference argument in the function.

Sure, let's unify.

hvdijk · 2025-06-02T13:12:09Z

As in #18565, this PR shows sycl/test/native_cpu/multiple_definitions.cpp being added but that doesn't belong in this PR, that came from #18726 and something went wrong in updating this PR.

Alexandr-Konovalov · 2025-06-02T13:19:58Z

As in #18565, this PR shows sycl/test/native_cpu/multiple_definitions.cpp being added but that doesn't belong in this PR, that came from #18726 and something went wrong in updating this PR.

Thank you, removed form this PR as well.

Alexandr-Konovalov · 2025-06-03T05:55:49Z

Colleagues @steffenlarsen , @Bensuo , could it be merged or not yet?

steffenlarsen · 2025-06-03T06:15:08Z

Definitely!

aelovikov-intel · 2025-08-25T17:24:15Z

sycl/include/sycl/detail/string_view.hpp

+  explicit operator std::string_view() const noexcept {
+#ifdef __INTEL_PREVIEW_BREAKING_CHANGES
+    return std::string_view(str, len);
+#else
+    return std::string_view(str);
+#endif
+  }


Shouldn't we pass data() instead of str here?

Alexandr-Konovalov added 3 commits May 26, 2025 10:45

[SYCL] Add size to detail::string_view

74b4758

This allows us to pass size of kernel name that is known in compile time, not compute the size in run time.

Merge branch 'sycl' into Alexandr-Konovalov/string_view-size

036b4a5

Code formatting.

34f2b76

Signed-off-by: Alexandr Konovalov <[email protected]>

Alexandr-Konovalov temporarily deployed to WindowsCILock May 26, 2025 10:16 — with GitHub Actions Inactive

Alexandr-Konovalov temporarily deployed to WindowsCILock May 26, 2025 10:41 — with GitHub Actions Inactive

frasercrmck mentioned this pull request May 26, 2025

[L0 v2] Flaky graph tests failing on Intel Arc A-Series Graphics #18668

Closed

frasercrmck mentioned this pull request May 26, 2025

[libspirv] Add include guards where appropriate #18666

Merged

Unify MKernelName usage.

88dcfda

Alexandr-Konovalov had a problem deploying to WindowsCILock May 27, 2025 14:34 — with GitHub Actions Failure

Code formatting.

d0914f4

Signed-off-by: Alexandr Konovalov <[email protected]>

Alexandr-Konovalov had a problem deploying to WindowsCILock May 27, 2025 14:45 — with GitHub Actions Error

Code formatting.

c60080e

Signed-off-by: Alexandr Konovalov <[email protected]>

Alexandr-Konovalov temporarily deployed to WindowsCILock May 27, 2025 14:53 — with GitHub Actions Inactive

Alexandr-Konovalov temporarily deployed to WindowsCILock May 27, 2025 15:21 — with GitHub Actions Inactive

Alexandr-Konovalov added 2 commits May 30, 2025 18:19

Next round of removing excessive strlen().

c64686e

Merge branch 'sycl' into Alexandr-Konovalov/string_view-size

f923082

Alexandr-Konovalov had a problem deploying to WindowsCILock May 30, 2025 16:34 — with GitHub Actions Error

Code formatting.

df1a8df

Signed-off-by: Alexandr Konovalov <[email protected]>

Alexandr-Konovalov temporarily deployed to WindowsCILock May 30, 2025 16:44 — with GitHub Actions Inactive

Alexandr-Konovalov temporarily deployed to WindowsCILock May 30, 2025 17:07 — with GitHub Actions Inactive

Alexandr-Konovalov marked this pull request as ready for review June 2, 2025 06:27

Alexandr-Konovalov requested review from a team as code owners June 2, 2025 06:27

Alexandr-Konovalov requested a review from Bensuo June 2, 2025 06:27

Alexandr-Konovalov requested a review from steffenlarsen June 2, 2025 06:27

steffenlarsen approved these changes Jun 2, 2025

View reviewed changes

Bensuo approved these changes Jun 2, 2025

View reviewed changes

Unify toKernelNameStrT().

5cefaf1

Alexandr-Konovalov requested a review from a team as a code owner June 2, 2025 13:06

Alexandr-Konovalov had a problem deploying to WindowsCILock June 2, 2025 13:06 — with GitHub Actions Error

Alexandr-Konovalov added 2 commits June 2, 2025 15:18

Remove sycl/test/native_cpu/multiple_definitions.cpp.

585dd61

Merge branch 'sycl' into Alexandr-Konovalov/string_view-size

0b1006d

Alexandr-Konovalov temporarily deployed to WindowsCILock June 2, 2025 13:19 — with GitHub Actions Inactive

Alexandr-Konovalov temporarily deployed to WindowsCILock June 2, 2025 13:50 — with GitHub Actions Inactive

Merge branch 'sycl' into Alexandr-Konovalov/string_view-size

a5b315c

Alexandr-Konovalov temporarily deployed to WindowsCILock June 2, 2025 14:49 — with GitHub Actions Inactive

Alexandr-Konovalov requested a deployment to WindowsCILock June 2, 2025 15:22 — with GitHub Actions In progress

Alexandr-Konovalov temporarily deployed to WindowsCILock June 2, 2025 15:22 — with GitHub Actions Inactive

Merge branch 'sycl' into Alexandr-Konovalov/string_view-size

4e5aa27

Alexandr-Konovalov temporarily deployed to WindowsCILock June 2, 2025 15:33 — with GitHub Actions Inactive

Alexandr-Konovalov temporarily deployed to WindowsCILock June 2, 2025 16:38 — with GitHub Actions Inactive

steffenlarsen merged commit 73ff319 into intel:sycl Jun 3, 2025
23 checks passed

Alexandr-Konovalov deleted the Alexandr-Konovalov/string_view-size branch August 20, 2025 14:48

aelovikov-intel reviewed Aug 25, 2025

View reviewed changes

[SYCL] Add size to detail::string_view #18661

[SYCL] Add size to detail::string_view #18661

Uh oh!

Conversation

Alexandr-Konovalov commented May 26, 2025

Uh oh!

frasercrmck commented May 26, 2025

Uh oh!

jbrodman commented May 27, 2025

Uh oh!

Alexandr-Konovalov commented May 28, 2025

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

steffenlarsen Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

Alexandr-Konovalov Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

steffenlarsen Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

Alexandr-Konovalov Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

hvdijk commented Jun 2, 2025

Uh oh!

Alexandr-Konovalov commented Jun 2, 2025

Uh oh!

Alexandr-Konovalov commented Jun 3, 2025

Uh oh!

steffenlarsen commented Jun 3, 2025

Uh oh!

Uh oh!

aelovikov-intel Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants