Skip to content

Conversation

@aelovikov-intel
Copy link
Contributor

Additional cleanups enabled by #19117.

Copy link
Member

@adamfidel adamfidel left a comment

Choose a reason for hiding this comment

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

Changes for SYCL Graph LGTM

aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Sep 3, 2025
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Sep 3, 2025
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Sep 3, 2025
aelovikov-intel added a commit that referenced this pull request Sep 4, 2025
@aelovikov-intel aelovikov-intel marked this pull request as draft September 4, 2025 22:40
@aelovikov-intel
Copy link
Contributor Author

I've forced pushed after rebasing on top of several smaller changes factored out from here. Doing the same via merges would have been much harder.

SYCL Graphs changes in this PR were not affected by the rebase.

@aelovikov-intel
Copy link
Contributor Author

aelovikov-intel commented Sep 8, 2025

getKernelName() is better be left to a subsequent PR, IMO. We have

#ifdef __INTEL_PREVIEW_BREAKING_CHANGES
using KernelNameStrT = std::string_view;
using KernelNameStrRefT = std::string_view;
using ABINeutralKernelNameStrT = detail::string_view;
#else
using KernelNameStrT = std::string;
using KernelNameStrRefT = const std::string &;
using ABINeutralKernelNameStrT = detail::string;
#endif

and some of our internal interfaces do expect const std::string & arguments. Creating those from std::string_view would result in otherwise unnecessary memory allocations.

On the other hand, I'd really prefer we do NOT introduce more ugly instances of KernelNameStrRefT so I'd rather keep getKernelName() returning std::string_view and use that when it's fine but leave explicit accesses to DeviceKernelInfo::Name when const std::string & might be needed.

Co-authored-by: Sergey Semenov <sergey.semenov@intel.com>
@aelovikov-intel
Copy link
Contributor Author

Jenkins failure (old RHEL/gcc) seems to have been caused by an earlier #19117, we probably had that part of CI not working back then.

@aelovikov-intel aelovikov-intel merged commit 6fc912a into intel:sycl Sep 9, 2025
57 of 62 checks passed
@aelovikov-intel aelovikov-intel deleted the device-kernel-info branch September 9, 2025 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants