Skip to content

Conversation

@vinser52
Copy link
Contributor

@vinser52 vinser52 commented May 5, 2025

Refactored the ProgramManager to use device_impl & instead of const device &.

See #18270 and #18251 that started the refactoring.

@vinser52 vinser52 requested a review from a team as a code owner May 5, 2025 16:26
@vinser52 vinser52 requested a review from sergey-semenov May 5, 2025 16:26
@vinser52 vinser52 changed the title Use refs to device_impl in ProgramManager [SYCL] Use refs to device_impl in ProgramManager May 5, 2025
@vinser52 vinser52 temporarily deployed to WindowsCILock May 5, 2025 16:26 — with GitHub Actions Inactive
@vinser52 vinser52 requested a review from aelovikov-intel May 5, 2025 16:27
ur::DeviceBinaryType Format = MainImg.getFormat();
if (!WorkList.empty() && !checkLinkingSupport(Dev, MainImg))
if (!WorkList.empty() &&
!checkLinkingSupport(*getSyclObjImpl(Dev).get(), MainImg))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why aren't we changing line 691 instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I didn't want to refactor the doesDevSupportDeviceRequirements function that internally calls the checkDevSupportDeviceRequirements that uses tempalte <typename Param> device::get_info<Param>

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I'll look into it separately.

const bundle_state ImgState = getBinImageState(BinImage);

assert(compatibleWithDevice(BinImage, Dev));
assert(compatibleWithDevice(BinImage, *getSyclObjImpl(Dev).get()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise, line 2554 would be a better change, IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about L2567 where we are creating vector of devices? I thoughts it also should be refactored, but in a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

What I've been doing during the transition is something like std::vector<device>{createSyclObjFromImpl<device>(Dev.shared_from_this())} but I can follow up on that separately.

@aelovikov-intel aelovikov-intel merged commit a36946d into intel:sycl May 5, 2025
27 of 28 checks passed
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.

2 participants