From 993aed35a6e3d14a80b4e644062167c898d5e6a5 Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Mon, 21 Jul 2025 11:32:35 -0700 Subject: [PATCH 01/31] program_manager.cpp: static create* --- .../detail/program_manager/program_manager.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index 485b94e36f658..a4fa824ea01f3 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -70,14 +70,14 @@ ProgramManager &ProgramManager::getInstance() { return GlobalHandler::instance().getProgramManager(); } -static ur_program_handle_t +static Managed createBinaryProgram(context_impl &Context, devices_range Devices, const uint8_t **Binaries, size_t *Lengths, const std::vector &Metadata) { assert(!Devices.empty() && "No devices provided for program creation"); adapter_impl &Adapter = Context.getAdapter(); - ur_program_handle_t Program; + Managed Program{Adapter}; auto DeviceHandles = Devices.to>(); ur_program_properties_t Properties = {}; Properties.stype = UR_STRUCTURE_TYPE_PROGRAM_PROPERTIES; @@ -92,11 +92,11 @@ createBinaryProgram(context_impl &Context, devices_range Devices, return Program; } -static ur_program_handle_t createSpirvProgram(context_impl &Context, +static Managed createSpirvProgram(context_impl &Context, const unsigned char *Data, size_t DataLen) { - ur_program_handle_t Program = nullptr; adapter_impl &Adapter = Context.getAdapter(); + Managed Program{Adapter}; Adapter.call(Context.getHandleRef(), Data, DataLen, nullptr, &Program); return Program; @@ -217,7 +217,7 @@ ProgramManager::createURProgram(const RTDeviceBinaryImage &Img, std::vector Binaries( Devices.size(), const_cast(RawImg.BinaryStart)); std::vector Lengths(Devices.size(), ImgSize); - ur_program_handle_t Res = + Managed Res = Format == SYCL_DEVICE_BINARY_TYPE_SPIRV ? createSpirvProgram(ContextImpl, RawImg.BinaryStart, ImgSize) : createBinaryProgram(ContextImpl, Devices, Binaries.data(), @@ -235,7 +235,7 @@ ProgramManager::createURProgram(const RTDeviceBinaryImage &Img, std::cerr << "created program: " << Res << "; image format: " << getFormatStr(Format) << "\n"; - return Res; + return Res.release(); } static void appendLinkOptionsFromImage(std::string &LinkOpts, @@ -518,7 +518,7 @@ std::pair ProgramManager::getOrCreateURProgram( ImgProgMetadata.begin(), ImgProgMetadata.end()); } NativePrg = createBinaryProgram(ContextImpl, Devices, BinPtrs.data(), - Lengths.data(), ProgMetadataVector); + Lengths.data(), ProgMetadataVector).release(); } else { NativePrg = createURProgram(MainImg, ContextImpl, Devices); } @@ -1244,8 +1244,8 @@ static bool loadDeviceLib(context_impl &Context, const char *Name, File.read(&FileContent[0], FileSize); File.close(); - Prog = - createSpirvProgram(Context, (unsigned char *)&FileContent[0], FileSize); + Prog = createSpirvProgram(Context, (unsigned char *)&FileContent[0], FileSize) + .release(); return Prog != nullptr; } From e18c0c1b56711ccbec9c561ad78d848582537446 Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Mon, 21 Jul 2025 12:05:00 -0700 Subject: [PATCH 02/31] ProgramManager::createURProgram --- .../program_manager/program_manager.cpp | 23 ++++++++++--------- .../program_manager/program_manager.hpp | 6 ++--- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index a4fa824ea01f3..9888c9b9f96f3 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -170,7 +170,7 @@ static bool isDeviceBinaryTypeSupported(context_impl &ContextImpl, return Out.str(); }; -ur_program_handle_t +Managed ProgramManager::createURProgram(const RTDeviceBinaryImage &Img, context_impl &ContextImpl, devices_range Devices) { @@ -235,7 +235,7 @@ ProgramManager::createURProgram(const RTDeviceBinaryImage &Img, std::cerr << "created program: " << Res << "; image format: " << getFormatStr(Format) << "\n"; - return Res.release(); + return Res; } static void appendLinkOptionsFromImage(std::string &LinkOpts, @@ -497,7 +497,7 @@ std::pair ProgramManager::getOrCreateURProgram( const std::vector &AllImages, context_impl &ContextImpl, devices_range Devices, const std::string &CompileAndLinkOptions, SerializedObj SpecConsts) { - ur_program_handle_t NativePrg; + Managed NativePrg; // Get binaries for each device (1:1 correpsondence with input Devices). auto Binaries = PersistentDeviceCodeCache::getItemFromDisc( @@ -518,11 +518,11 @@ std::pair ProgramManager::getOrCreateURProgram( ImgProgMetadata.begin(), ImgProgMetadata.end()); } NativePrg = createBinaryProgram(ContextImpl, Devices, BinPtrs.data(), - Lengths.data(), ProgMetadataVector).release(); + Lengths.data(), ProgMetadataVector); } else { NativePrg = createURProgram(MainImg, ContextImpl, Devices); } - return {NativePrg, Binaries.size()}; + return {NativePrg.release(), Binaries.size()}; } /// Emits information about built programs if the appropriate contitions are @@ -947,8 +947,8 @@ ProgramManager::getBuiltURProgram(const BinImgWithDeps &ImgWithDeps, if (UseDeviceLibs) DeviceLibReqMask |= getDeviceLibReqMask(*BinImg); - Managed NativePrg{ - createURProgram(*BinImg, ContextImpl, Devs), Adapter}; + Managed NativePrg = + createURProgram(*BinImg, ContextImpl, Devs); if (BinImg->supportsSpecConstants()) { enableITTAnnotationsIfNeeded(NativePrg, Adapter); @@ -2833,7 +2833,7 @@ ProgramManager::compile(const DevImgPlainWithDeps &ImgWithDeps, adapter_impl &Adapter = getSyclObjImpl(InputImpl.get_context())->getAdapter(); - ur_program_handle_t Prog = + Managed Prog = createURProgram(*InputImpl.get_bin_image_ref(), *getSyclObjImpl(InputImpl.get_context()), Devs); @@ -2848,7 +2848,7 @@ ProgramManager::compile(const DevImgPlainWithDeps &ImgWithDeps, InputImpl.getRTCInfo(); DeviceImageImplPtr ObjectImpl = device_image_impl::create( InputImpl.get_bin_image_ref(), InputImpl.get_context(), Devs, - bundle_state::object, InputImpl.get_kernel_ids_ptr(), Prog, + bundle_state::object, InputImpl.get_kernel_ids_ptr(), Prog.release(), InputImpl.get_spec_const_data_ref(), InputImpl.get_spec_const_blob_ref(), InputImpl.getOriginMask(), std::move(RTCInfo), std::move(KernelNames), @@ -3247,10 +3247,11 @@ ur_kernel_handle_t ProgramManager::getOrCreateMaterializedKernel( if constexpr (DbgProgMgr > 0) std::cerr << ">>> Adding the kernel to the cache.\n"; context_impl &ContextImpl = *detail::getSyclObjImpl(Context); - auto Program = createURProgram(Img, ContextImpl, {Device}); detail::device_impl &DeviceImpl = *detail::getSyclObjImpl(Device); adapter_impl &Adapter = DeviceImpl.getAdapter(); - Managed ProgramManaged(Program, Adapter); + + Managed ProgramManaged = + createURProgram(Img, ContextImpl, {Device}); std::string CompileOpts; std::string LinkOpts; diff --git a/sycl/source/detail/program_manager/program_manager.hpp b/sycl/source/detail/program_manager/program_manager.hpp index 221abed5af865..7bc69d3497074 100644 --- a/sycl/source/detail/program_manager/program_manager.hpp +++ b/sycl/source/detail/program_manager/program_manager.hpp @@ -143,9 +143,9 @@ class ProgramManager { const std::unordered_set &ImagesToVerify, context_impl &ContextImpl, const device_impl &DeviceImpl); - ur_program_handle_t createURProgram(const RTDeviceBinaryImage &Img, - context_impl &ContextImpl, - devices_range Devices); + Managed createURProgram(const RTDeviceBinaryImage &Img, + context_impl &ContextImpl, + devices_range Devices); /// Creates a UR program using either a cached device code binary if present /// in the persistent cache or from the supplied device image otherwise. /// \param Img The device image used to create the program. From 5d209d3da91a5dafb2bbc7c4f0be6561f0676bb4 Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Mon, 21 Jul 2025 12:15:24 -0700 Subject: [PATCH 03/31] program_manager.cpp: loadDeviceLib --- .../program_manager/program_manager.cpp | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index 9888c9b9f96f3..01022608e7ddd 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -1228,13 +1228,13 @@ ProgramManager::getProgramBuildLog(const ur_program_handle_t &Program, // TODO device libraries may use scpecialization constants, manifest files, etc. // To support that they need to be delivered in a different container - so that // sycl_device_binary_struct can be created for each of them. -static bool loadDeviceLib(context_impl &Context, const char *Name, - ur_program_handle_t &Prog) { +static Managed loadDeviceLib(context_impl &Context, + const char *Name) { std::string LibSyclDir = OSUtil::getCurrentDSODir(); std::ifstream File(LibSyclDir + OSUtil::DirSep + Name, std::ifstream::in | std::ifstream::binary); if (!File.good()) { - return false; + return {}; } File.seekg(0, std::ios::end); @@ -1244,9 +1244,8 @@ static bool loadDeviceLib(context_impl &Context, const char *Name, File.read(&FileContent[0], FileSize); File.close(); - Prog = createSpirvProgram(Context, (unsigned char *)&FileContent[0], FileSize) - .release(); - return Prog != nullptr; + return createSpirvProgram(Context, (unsigned char *)&FileContent[0], + FileSize); } // For each extension, a pair of library names. The first uses native support, @@ -1367,10 +1366,17 @@ loadDeviceLibFallback(context_impl &Context, DeviceLibExt Extension, bool IsProgramCreated = !URProgram; // Create UR program for device lib if we don't have it yet. - if (!URProgram && !loadDeviceLib(Context, LibFileName, URProgram)) { - EraseProgramForDevices(); - throw exception(make_error_code(errc::build), - std::string("Failed to load ") + LibFileName); + if (!URProgram) { + Managed DeviceLibProgram = + loadDeviceLib(Context, LibFileName); + if (DeviceLibProgram == nullptr) { + EraseProgramForDevices(); + throw exception(make_error_code(errc::build), + std::string("Failed to load ") + LibFileName); + } + + // TODO: How isn't this a leak? + URProgram = DeviceLibProgram.release(); } // Insert URProgram into the cache for all devices that we compiled it for. From 11ffadc710e4df2b0139bb95f3f52a8f4206db9c Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Mon, 21 Jul 2025 12:50:36 -0700 Subject: [PATCH 04/31] ProgramManager::getOrCreateURProgram --- .../program_manager/program_manager.cpp | 46 +++++++++---------- .../program_manager/program_manager.hpp | 2 +- 2 files changed, 23 insertions(+), 25 deletions(-) diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index 01022608e7ddd..0576cd8b14a96 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -492,7 +492,8 @@ static void applyOptionsFromEnvironment(std::string &CompileOpts, applyLinkOptionsFromEnvironment(LinkOpts); } -std::pair ProgramManager::getOrCreateURProgram( +std::pair, bool> +ProgramManager::getOrCreateURProgram( const RTDeviceBinaryImage &MainImg, const std::vector &AllImages, context_impl &ContextImpl, devices_range Devices, @@ -502,27 +503,26 @@ std::pair ProgramManager::getOrCreateURProgram( // Get binaries for each device (1:1 correpsondence with input Devices). auto Binaries = PersistentDeviceCodeCache::getItemFromDisc( Devices, AllImages, SpecConsts, CompileAndLinkOptions); - if (!Binaries.empty()) { - std::vector BinPtrs; - std::vector Lengths; - for (auto &Bin : Binaries) { - Lengths.push_back(Bin.size()); - BinPtrs.push_back(reinterpret_cast(Bin.data())); - } - - // Get program metadata from properties - std::vector ProgMetadataVector; - for (const RTDeviceBinaryImage *Img : AllImages) { - auto &ImgProgMetadata = Img->getProgramMetadataUR(); - ProgMetadataVector.insert(ProgMetadataVector.end(), - ImgProgMetadata.begin(), ImgProgMetadata.end()); - } - NativePrg = createBinaryProgram(ContextImpl, Devices, BinPtrs.data(), - Lengths.data(), ProgMetadataVector); - } else { - NativePrg = createURProgram(MainImg, ContextImpl, Devices); + if (Binaries.empty()) + return {createURProgram(MainImg, ContextImpl, Devices), false}; + + std::vector BinPtrs; + std::vector Lengths; + for (auto &Bin : Binaries) { + Lengths.push_back(Bin.size()); + BinPtrs.push_back(reinterpret_cast(Bin.data())); + } + + // Get program metadata from properties + std::vector ProgMetadataVector; + for (const RTDeviceBinaryImage *Img : AllImages) { + auto &ImgProgMetadata = Img->getProgramMetadataUR(); + ProgMetadataVector.insert(ProgMetadataVector.end(), ImgProgMetadata.begin(), + ImgProgMetadata.end()); } - return {NativePrg.release(), Binaries.size()}; + return {createBinaryProgram(ContextImpl, Devices, BinPtrs.data(), + Lengths.data(), ProgMetadataVector), + true}; } /// Emits information about built programs if the appropriate contitions are @@ -920,8 +920,6 @@ ProgramManager::getBuiltURProgram(const BinImgWithDeps &ImgWithDeps, NativePrg, Adapter); } - Managed ProgramManaged(NativePrg, Adapter); - // Link a fallback implementation of device libraries if they are not // supported by a device compiler. // Pre-compiled programs (after AOT compilation or read from persitent @@ -964,7 +962,7 @@ ProgramManager::getBuiltURProgram(const BinImgWithDeps &ImgWithDeps, auto URDevices = Devs.to>(); Managed BuiltProgram = - build(std::move(ProgramManaged), ContextImpl, CompileOpts, LinkOpts, + build(std::move(NativePrg), ContextImpl, CompileOpts, LinkOpts, URDevices, DeviceLibReqMask, ProgramsToLink, /*CreatedFromBinary*/ MainImg.getFormat() != SYCL_DEVICE_BINARY_TYPE_SPIRV); diff --git a/sycl/source/detail/program_manager/program_manager.hpp b/sycl/source/detail/program_manager/program_manager.hpp index 7bc69d3497074..d02cb951024e4 100644 --- a/sycl/source/detail/program_manager/program_manager.hpp +++ b/sycl/source/detail/program_manager/program_manager.hpp @@ -165,7 +165,7 @@ class ProgramManager { /// \return A pair consisting of the UR program created with the corresponding /// device code binary and a boolean that is true if the device code /// binary was found in the persistent cache and false otherwise. - std::pair getOrCreateURProgram( + std::pair, bool> getOrCreateURProgram( const RTDeviceBinaryImage &Img, const std::vector &AllImages, context_impl &ContextImpl, devices_range Devices, From 0ea85a3b3db86525be32338b151816845d1f8882 Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Mon, 21 Jul 2025 13:06:30 -0700 Subject: [PATCH 05/31] BuildF lambda, hopefully... --- sycl/source/detail/kernel_program_cache.hpp | 6 +++++- sycl/source/detail/program_manager/program_manager.cpp | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/sycl/source/detail/kernel_program_cache.hpp b/sycl/source/detail/kernel_program_cache.hpp index 8e9fa563f8874..aaf3d8f390ee7 100644 --- a/sycl/source/detail/kernel_program_cache.hpp +++ b/sycl/source/detail/kernel_program_cache.hpp @@ -801,7 +801,11 @@ class KernelProgramCache { try { // Remove `adapter_impl` from `ProgramBuildResult`'s ctors once `Build` // returns `ManagedVal) = Build(); + + static_assert( + std::is_same_vVal)>, + "Are we casting from Managed to plain URResource?"); + BuildResult->Val = Build(); if constexpr (!std::is_same_v) EvictFunc(BuildResult->Val, /*IsBuilt=*/true); diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index 0576cd8b14a96..c44d11e82185d 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -994,11 +994,11 @@ ProgramManager::getBuiltURProgram(const BinImgWithDeps &ImgWithDeps, BuiltProgram); } - return BuiltProgram.release(); + return BuiltProgram; }; if (!SYCLConfig::get()) - return BuildF(); + return BuildF().release(); uint32_t ImgId = ImgWithDeps.getMain()->getImageID(); std::set URDevicesSet; From 7d7531a121ee69c0d8992c9ddf1cc6c427b1d5da Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Mon, 21 Jul 2025 13:23:07 -0700 Subject: [PATCH 06/31] FastKernelCacheVal ctor --- sycl/source/detail/kernel_name_based_cache_t.hpp | 5 +++-- .../source/detail/program_manager/program_manager.cpp | 11 ++++++----- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/sycl/source/detail/kernel_name_based_cache_t.hpp b/sycl/source/detail/kernel_name_based_cache_t.hpp index ef92112dcf98e..0020a1ff6352b 100644 --- a/sycl/source/detail/kernel_name_based_cache_t.hpp +++ b/sycl/source/detail/kernel_name_based_cache_t.hpp @@ -36,9 +36,10 @@ struct FastKernelCacheVal { FastKernelCacheVal(ur_kernel_handle_t KernelHandle, std::mutex *Mutex, const KernelArgMask *KernelArgMask, - ur_program_handle_t ProgramHandle, adapter_impl &Adapter) + Managed &&ProgramHandle, + adapter_impl &Adapter) : MKernelHandle(KernelHandle), MMutex(Mutex), - MKernelArgMask(KernelArgMask), MProgramHandle(ProgramHandle, Adapter), + MKernelArgMask(KernelArgMask), MProgramHandle(std::move(ProgramHandle)), MAdapter(Adapter) {} ~FastKernelCacheVal() { diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index c44d11e82185d..93ce20011779f 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -1110,8 +1110,9 @@ FastKernelCacheValPtr ProgramManager::getOrCreateKernel( } } - ur_program_handle_t Program = - getBuiltURProgram(ContextImpl, DeviceImpl, KernelName, NDRDesc); + Managed Program{ + getBuiltURProgram(ContextImpl, DeviceImpl, KernelName, NDRDesc), + ContextImpl.getAdapter()}; auto BuildF = [this, &Program, &KernelName, &ContextImpl] { ur_kernel_handle_t Kernel = nullptr; @@ -1136,7 +1137,7 @@ FastKernelCacheValPtr ProgramManager::getOrCreateKernel( return std::make_pair(Kernel, ArgMask); }; - auto GetCachedBuildF = [&Cache, &KernelName, Program]() { + auto GetCachedBuildF = [&Cache, &KernelName, &Program]() { return Cache.getOrInsertKernel(Program, KernelName); }; @@ -1146,7 +1147,7 @@ FastKernelCacheValPtr ProgramManager::getOrCreateKernel( // nullptr for the mutex. auto [Kernel, ArgMask] = BuildF(); return std::make_shared( - Kernel, nullptr, ArgMask, Program, ContextImpl.getAdapter()); + Kernel, nullptr, ArgMask, std::move(Program), ContextImpl.getAdapter()); } std::shared_ptr BuildResult = @@ -1156,7 +1157,7 @@ FastKernelCacheValPtr ProgramManager::getOrCreateKernel( &KernelArgMaskPair = BuildResult->Val; auto ret_val = std::make_shared( KernelArgMaskPair.first, &(BuildResult->MBuildResultMutex), - KernelArgMaskPair.second, Program, ContextImpl.getAdapter()); + KernelArgMaskPair.second, std::move(Program), ContextImpl.getAdapter()); // If caching is enabled, one copy of the kernel handle will be // stored in FastKernelCacheVal, and one is in // KernelProgramCache::MKernelsPerProgramCache. To cover From 6e3345dc30b980a17ea679489b008b415f097d56 Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Mon, 21 Jul 2025 14:19:15 -0700 Subject: [PATCH 07/31] extKernelCompilerFetchFromCache --- sycl/source/detail/device_image_impl.hpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/sycl/source/detail/device_image_impl.hpp b/sycl/source/detail/device_image_impl.hpp index 1caf13667b26f..51cb665469d33 100644 --- a/sycl/source/detail/device_image_impl.hpp +++ b/sycl/source/detail/device_image_impl.hpp @@ -774,11 +774,12 @@ class device_image_impl ur_program_handle_t UrProgram = nullptr; // SourceStrPtr will be null when source is Spir-V bytes. const std::string *SourceStrPtr = std::get_if(&MBinImage); - bool FetchedFromCache = false; if (PersistentDeviceCodeCache::isEnabled() && SourceStrPtr) { - FetchedFromCache = extKernelCompilerFetchFromCache( - Devices, BuildOptions, *SourceStrPtr, UrProgram); + UrProgram = + extKernelCompilerFetchFromCache(Devices, BuildOptions, *SourceStrPtr) + .release(); } + bool FetchedFromCache = (UrProgram != nullptr); adapter_impl &Adapter = ContextImpl.getAdapter(); @@ -907,10 +908,10 @@ class device_image_impl return SS.str(); } - bool extKernelCompilerFetchFromCache( + Managed extKernelCompilerFetchFromCache( devices_range Devices, const std::vector &BuildOptions, - const std::string &SourceStr, ur_program_handle_t &UrProgram) const { + const std::string &SourceStr) const { sycl::detail::context_impl &ContextImpl = *getSyclObjImpl(MContext); adapter_impl &Adapter = ContextImpl.getAdapter(); @@ -924,7 +925,7 @@ class device_image_impl PersistentDeviceCodeCache::getCompiledKernelFromDisc(Devices, UserArgs, SourceStr); if (BinProgs.empty()) { - return false; + return {}; } for (auto &BinProg : BinProgs) { Binaries.push_back((uint8_t *)(BinProg.data())); @@ -937,11 +938,12 @@ class device_image_impl Properties.count = 0; Properties.pMetadatas = nullptr; + Managed UrProgram{Adapter}; Adapter.call( ContextImpl.getHandleRef(), DeviceHandles.size(), DeviceHandles.data(), Lengths.data(), Binaries.data(), &Properties, &UrProgram); - return true; + return UrProgram; } // Get the specialization constant default value blob. From 996206848670582d90886f1d5420b4bfe5805c95 Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Mon, 21 Jul 2025 14:23:11 -0700 Subject: [PATCH 08/31] createProgramFromSource --- sycl/source/detail/device_image_impl.hpp | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/sycl/source/detail/device_image_impl.hpp b/sycl/source/detail/device_image_impl.hpp index 51cb665469d33..9e231d879971e 100644 --- a/sycl/source/detail/device_image_impl.hpp +++ b/sycl/source/detail/device_image_impl.hpp @@ -771,13 +771,12 @@ class device_image_impl auto DeviceVec = Devices.to>(); - ur_program_handle_t UrProgram = nullptr; + Managed UrProgram; // SourceStrPtr will be null when source is Spir-V bytes. const std::string *SourceStrPtr = std::get_if(&MBinImage); if (PersistentDeviceCodeCache::isEnabled() && SourceStrPtr) { UrProgram = - extKernelCompilerFetchFromCache(Devices, BuildOptions, *SourceStrPtr) - .release(); + extKernelCompilerFetchFromCache(Devices, BuildOptions, *SourceStrPtr); } bool FetchedFromCache = (UrProgram != nullptr); @@ -814,7 +813,7 @@ class device_image_impl } return std::vector>{ device_image_impl::create(MContext, Devices, bundle_state::executable, - UrProgram, MRTCBinInfo->MLanguage, + UrProgram.release(), MRTCBinInfo->MLanguage, std::move(KernelNameSet))}; } @@ -1228,7 +1227,7 @@ class device_image_impl return Result; } - ur_program_handle_t + Managed createProgramFromSource(devices_range Devices, const std::vector &Options, std::string *LogPtr) const { @@ -1268,11 +1267,10 @@ class device_image_impl "languages at this time"); }(); - ur_program_handle_t UrProgram = nullptr; + Managed UrProgram{Adapter}; Adapter.call(ContextImpl.getHandleRef(), spirv.data(), spirv.size(), nullptr, &UrProgram); - // program created by urProgramCreateWithIL is implicitly retained. if (UrProgram == nullptr) throw sycl::exception( sycl::make_error_code(errc::invalid), From e9feebfc017a0b7a130cac56c21294878596ee27 Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Mon, 21 Jul 2025 14:31:13 -0700 Subject: [PATCH 09/31] device_image_impl ctor overload 0 --- sycl/source/backend.cpp | 7 ++++--- sycl/source/detail/device_image_impl.hpp | 4 ++-- sycl/source/detail/kernel_bundle_impl.hpp | 3 ++- sycl/source/detail/program_manager/program_manager.cpp | 10 +++++----- 4 files changed, 13 insertions(+), 11 deletions(-) diff --git a/sycl/source/backend.cpp b/sycl/source/backend.cpp index eaa558ef8bc35..1c6ec1ad3ae5d 100644 --- a/sycl/source/backend.cpp +++ b/sycl/source/backend.cpp @@ -301,9 +301,10 @@ make_kernel_bundle(ur_native_handle_t NativeHandle, // do the same to user images, since they may contain references to undefined // symbols (e.g. when kernel_bundle is supposed to be joined with another). auto KernelIDs = std::make_shared>(); - auto DevImgImpl = - device_image_impl::create(nullptr, TargetContext, Devices, State, - KernelIDs, UrProgram, ImageOriginInterop); + auto DevImgImpl = device_image_impl::create( + nullptr, TargetContext, Devices, State, KernelIDs, + // TODO: Move creation of `Managed` up. + Managed{UrProgram, Adapter}, ImageOriginInterop); device_image_plain DevImg{DevImgImpl}; return kernel_bundle_impl::create(TargetContext, Devices, DevImg); diff --git a/sycl/source/detail/device_image_impl.hpp b/sycl/source/detail/device_image_impl.hpp index 9e231d879971e..66d4b8d99235a 100644 --- a/sycl/source/detail/device_image_impl.hpp +++ b/sycl/source/detail/device_image_impl.hpp @@ -257,10 +257,10 @@ class device_image_impl device_image_impl(const RTDeviceBinaryImage *BinImage, context Context, devices_range Devices, bundle_state State, std::shared_ptr> KernelIDs, - ur_program_handle_t Program, uint8_t Origins, private_tag) + Managed &&Program, uint8_t Origins, private_tag) : MBinImage(BinImage), MContext(std::move(Context)), MDevices(Devices.to>()), MState(State), - MProgram(Program, getSyclObjImpl(MContext)->getAdapter()), + MProgram(std::move(Program)), MKernelIDs(std::move(KernelIDs)), MSpecConstsDefValBlob(getSpecConstsDefValBlob()), MOrigins(Origins) { updateSpecConstSymMap(); diff --git a/sycl/source/detail/kernel_bundle_impl.hpp b/sycl/source/detail/kernel_bundle_impl.hpp index aef841cc3f3b8..22cd8d8fd9d72 100644 --- a/sycl/source/detail/kernel_bundle_impl.hpp +++ b/sycl/source/detail/kernel_bundle_impl.hpp @@ -597,7 +597,8 @@ class kernel_bundle_impl for (const detail::RTDeviceBinaryImage *Image : BestImages) MDeviceImages.emplace_back(device_image_impl::create( Image, Context, Devs, ProgramManager::getBinImageState(Image), - /*KernelIDs=*/nullptr, /*URProgram=*/nullptr, ImageOriginSYCLBIN)); + /*KernelIDs=*/nullptr, Managed{}, + ImageOriginSYCLBIN)); ProgramManager::getInstance().bringSYCLDeviceImagesToState(MDeviceImages, State); fillUniqueDeviceImages(); diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index 93ce20011779f..0abc86a7de459 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -2481,9 +2481,9 @@ device_image_plain ProgramManager::getDeviceImageFromBinaryImage( KernelIDs = m_BinImg2KernelIDs[BinImage]; } - DeviceImageImplPtr Impl = - device_image_impl::create(BinImage, Ctx, Dev, ImgState, KernelIDs, - /*PIProgram=*/nullptr, ImageOriginSYCLOffline); + DeviceImageImplPtr Impl = device_image_impl::create( + BinImage, Ctx, Dev, ImgState, KernelIDs, Managed{}, + ImageOriginSYCLOffline); return createSyclObjFromImpl(std::move(Impl)); } @@ -2645,7 +2645,7 @@ ProgramManager::getSYCLDeviceImagesWithCompatibleState( DeviceImageImplPtr MainImpl = device_image_impl::create( ImgInfoPair.first, Ctx, Devs, ImgInfoPair.second.State, - ImgInfoPair.second.KernelIDs, /*PIProgram=*/nullptr, + ImgInfoPair.second.KernelIDs, Managed{}, ImageOriginSYCLOffline); std::vector Images; @@ -2680,7 +2680,7 @@ ProgramManager::createDependencyImage(const context &Ctx, devices_range Devs, "State mismatch between main image and its dependency"); DeviceImageImplPtr DepImpl = device_image_impl::create( DepImage, Ctx, Devs, DepState, std::move(DepKernelIDs), - /*PIProgram=*/nullptr, ImageOriginSYCLOffline); + Managed{}, ImageOriginSYCLOffline); return createSyclObjFromImpl(std::move(DepImpl)); } From af403f03169073020aa9a7fee43b92744412f55e Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Mon, 21 Jul 2025 14:40:12 -0700 Subject: [PATCH 10/31] device_image_impl ctor overload 1 --- sycl/source/detail/device_image_impl.hpp | 6 +++--- .../detail/program_manager/program_manager.cpp | 18 +++++++++++------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/sycl/source/detail/device_image_impl.hpp b/sycl/source/detail/device_image_impl.hpp index 66d4b8d99235a..8c1afc496fc9a 100644 --- a/sycl/source/detail/device_image_impl.hpp +++ b/sycl/source/detail/device_image_impl.hpp @@ -287,7 +287,7 @@ class device_image_impl const RTDeviceBinaryImage *BinImage, const context &Context, devices_range Devices, bundle_state State, std::shared_ptr> KernelIDs, - ur_program_handle_t Program, const SpecConstMapT &SpecConstMap, + Managed &&Program, const SpecConstMapT &SpecConstMap, const std::vector &SpecConstsBlob, uint8_t Origins, std::optional &&RTCInfo, KernelNameSetT &&KernelNames, @@ -295,8 +295,8 @@ class device_image_impl std::unique_ptr &&MergedImageStorage, private_tag) : MBinImage(BinImage), MContext(std::move(Context)), MDevices(Devices.to>()), MState(State), - MProgram(Program, getSyclObjImpl(MContext)->getAdapter()), - MKernelIDs(std::move(KernelIDs)), MKernelNames{std::move(KernelNames)}, + MProgram(std::move(Program)), MKernelIDs(std::move(KernelIDs)), + MKernelNames{std::move(KernelNames)}, MEliminatedKernelArgMasks{std::move(EliminatedKernelArgMasks)}, MSpecConstsBlob(SpecConstsBlob), MSpecConstsDefValBlob(getSpecConstsDefValBlob()), diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index 0abc86a7de459..3e209b3ce384c 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -2853,7 +2853,7 @@ ProgramManager::compile(const DevImgPlainWithDeps &ImgWithDeps, InputImpl.getRTCInfo(); DeviceImageImplPtr ObjectImpl = device_image_impl::create( InputImpl.get_bin_image_ref(), InputImpl.get_context(), Devs, - bundle_state::object, InputImpl.get_kernel_ids_ptr(), Prog.release(), + bundle_state::object, InputImpl.get_kernel_ids_ptr(), std::move(Prog), InputImpl.get_spec_const_data_ref(), InputImpl.get_spec_const_blob_ref(), InputImpl.getOriginMask(), std::move(RTCInfo), std::move(KernelNames), @@ -3051,8 +3051,10 @@ ProgramManager::link(const std::vector &Imgs, DeviceImageImplPtr ExecutableImpl = device_image_impl::create( NewBinImg, Context, Devs, bundle_state::executable, std::move(KernelIDs), - LinkedProg, std::move(NewSpecConstMap), std::move(NewSpecConstBlob), - CombinedOrigins, std::move(MergedRTCInfo), std::move(MergedKernelNames), + // TODO: Move creation of `Managed` up: + Managed{LinkedProg, Adapter}, + std::move(NewSpecConstMap), std::move(NewSpecConstBlob), CombinedOrigins, + std::move(MergedRTCInfo), std::move(MergedKernelNames), std::move(MergedEliminatedKernelArgMasks), std::move(MergedImageStorage)); // TODO: Make multiple sets of device images organized by devices they are @@ -3133,10 +3135,12 @@ ProgramManager::build(const DevImgPlainWithDeps &DevImgWithDeps, DeviceImageImplPtr ExecImpl = device_image_impl::create( ResultBinImg, Context, Devs, bundle_state::executable, - std::move(KernelIDs), ResProgram, std::move(SpecConstMap), - std::move(SpecConstBlob), CombinedOrigins, std::move(MergedRTCInfo), - std::move(MergedKernelNames), std::move(MergedEliminatedKernelArgMasks), - std::move(MergedImageStorage)); + std::move(KernelIDs), + // Move creation of `Managed` up: + Managed{ResProgram, ContextImpl.getAdapter()}, + std::move(SpecConstMap), std::move(SpecConstBlob), CombinedOrigins, + std::move(MergedRTCInfo), std::move(MergedKernelNames), + std::move(MergedEliminatedKernelArgMasks), std::move(MergedImageStorage)); return createSyclObjFromImpl(std::move(ExecImpl)); } From dd34ba56e68b9e6dbb3718413741cd72af8b040d Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Mon, 21 Jul 2025 14:43:15 -0700 Subject: [PATCH 11/31] Drop unused device_image_impl ctor overload 2 --- sycl/source/detail/device_image_impl.hpp | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/sycl/source/detail/device_image_impl.hpp b/sycl/source/detail/device_image_impl.hpp index 8c1afc496fc9a..dc4b31fba2c1d 100644 --- a/sycl/source/detail/device_image_impl.hpp +++ b/sycl/source/detail/device_image_impl.hpp @@ -304,23 +304,6 @@ class device_image_impl MRTCBinInfo(std::move(RTCInfo)), MMergedImageStorage(std::move(MergedImageStorage)) {} - device_image_impl(const RTDeviceBinaryImage *BinImage, const context &Context, - devices_range Devices, bundle_state State, - ur_program_handle_t Program, syclex::source_language Lang, - KernelNameSetT &&KernelNames, - KernelNameToArgMaskMap &&EliminatedKernelArgMasks, - private_tag) - : MBinImage(BinImage), MContext(std::move(Context)), - MDevices(Devices.to>()), MState(State), - MProgram(Program, getSyclObjImpl(MContext)->getAdapter()), - MKernelNames{std::move(KernelNames)}, - MEliminatedKernelArgMasks{std::move(EliminatedKernelArgMasks)}, - MSpecConstsDefValBlob(getSpecConstsDefValBlob()), - MOrigins(ImageOriginKernelCompiler), - MRTCBinInfo(KernelCompilerBinaryInfo{Lang}) { - updateSpecConstSymMap(); - } - device_image_impl( const RTDeviceBinaryImage *BinImage, const context &Context, devices_range Devices, bundle_state State, From da04557200b58146cea746e153dbca9fab427344 Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Mon, 21 Jul 2025 14:48:14 -0700 Subject: [PATCH 12/31] Last affected device_image_impl ctor overload --- sycl/source/detail/device_image_impl.hpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/sycl/source/detail/device_image_impl.hpp b/sycl/source/detail/device_image_impl.hpp index dc4b31fba2c1d..d050d9b76bf66 100644 --- a/sycl/source/detail/device_image_impl.hpp +++ b/sycl/source/detail/device_image_impl.hpp @@ -349,14 +349,13 @@ class device_image_impl } device_image_impl(const context &Context, devices_range Devices, - bundle_state State, ur_program_handle_t Program, + bundle_state State, Managed Program, syclex::source_language Lang, KernelNameSetT &&KernelNames, private_tag) : MBinImage(static_cast(nullptr)), MContext(std::move(Context)), MDevices(Devices.to>()), MState(State), - MProgram(Program, getSyclObjImpl(MContext)->getAdapter()), - MKernelNames{std::move(KernelNames)}, + MProgram(std::move(Program)), MKernelNames{std::move(KernelNames)}, MSpecConstsDefValBlob(getSpecConstsDefValBlob()), MOrigins(ImageOriginKernelCompiler), MRTCBinInfo(KernelCompilerBinaryInfo{Lang}) {} @@ -796,7 +795,7 @@ class device_image_impl } return std::vector>{ device_image_impl::create(MContext, Devices, bundle_state::executable, - UrProgram.release(), MRTCBinInfo->MLanguage, + std::move(UrProgram), MRTCBinInfo->MLanguage, std::move(KernelNameSet))}; } From 5c57574eb25fefbc2b31f2627f1b53cc5b92af5b Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Mon, 21 Jul 2025 15:35:09 -0700 Subject: [PATCH 13/31] Move up --- .../source/detail/program_manager/program_manager.cpp | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index 3e209b3ce384c..e1956a1b84f5f 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -2973,7 +2973,7 @@ ProgramManager::link(const std::vector &Imgs, context_impl &ContextImpl = *getSyclObjImpl(Context); adapter_impl &Adapter = ContextImpl.getAdapter(); - ur_program_handle_t LinkedProg = nullptr; + Managed LinkedProg{Adapter}; auto doLink = [&] { auto Res = Adapter.call_nocheck( ContextImpl.getHandleRef(), URDevices.size(), URDevices.data(), @@ -3051,11 +3051,10 @@ ProgramManager::link(const std::vector &Imgs, DeviceImageImplPtr ExecutableImpl = device_image_impl::create( NewBinImg, Context, Devs, bundle_state::executable, std::move(KernelIDs), - // TODO: Move creation of `Managed` up: - Managed{LinkedProg, Adapter}, - std::move(NewSpecConstMap), std::move(NewSpecConstBlob), CombinedOrigins, - std::move(MergedRTCInfo), std::move(MergedKernelNames), - std::move(MergedEliminatedKernelArgMasks), std::move(MergedImageStorage)); + std::move(LinkedProg), std::move(NewSpecConstMap), + std::move(NewSpecConstBlob), CombinedOrigins, std::move(MergedRTCInfo), + std::move(MergedKernelNames), std::move(MergedEliminatedKernelArgMasks), + std::move(MergedImageStorage)); // TODO: Make multiple sets of device images organized by devices they are // compiled for. From 1799d4f9e2c27ab2439afa1ba7f45ed70dd54c09 Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Mon, 21 Jul 2025 15:48:58 -0700 Subject: [PATCH 14/31] Debug prints --- sycl/source/detail/adapter_impl.hpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/sycl/source/detail/adapter_impl.hpp b/sycl/source/detail/adapter_impl.hpp index 639eff7ab702d..6aa48f4d43a25 100644 --- a/sycl/source/detail/adapter_impl.hpp +++ b/sycl/source/detail/adapter_impl.hpp @@ -247,7 +247,9 @@ template class Managed { public: Managed() = default; - Managed(URResource R, adapter_impl &Adapter) : R(R), Adapter(&Adapter) {} + Managed(URResource R, adapter_impl &Adapter) : R(R), Adapter(&Adapter) { + std::cerr << "Ctor created: " << R << std::endl; + } Managed(adapter_impl &Adapter) : Adapter(&Adapter) {} Managed(const Managed &) = delete; Managed(Managed &&Other) : Adapter(Other.Adapter) { @@ -256,8 +258,10 @@ template class Managed { } Managed &operator=(const Managed &) = delete; Managed &operator=(Managed &&Other) { - if (R) + if (R) { + std::cerr << "Assign releasing: " << R << std::endl; Adapter->call(R); + } R = Other.R; Other.R = nullptr; Adapter = Other.Adapter; @@ -267,6 +271,7 @@ template class Managed { operator URResource() const { return R; } URResource release() { + std::cerr << "Manually releasing: " << R << std::endl; URResource Res = R; R = nullptr; return Res; @@ -275,6 +280,7 @@ template class Managed { URResource *operator&() { assert(!R && "Already initialized!"); assert(Adapter && "Adapter must be set for this API!"); + std::cerr << "Creating externally..." << std::endl; return &R; } @@ -282,6 +288,7 @@ template class Managed { if (!R) return; + std::cerr << "Dtor releasing: " << R << std::endl; Adapter->call(R); } From fe03fa844b50da5907ec40b6cf5dbbd12b0e569a Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Mon, 21 Jul 2025 16:06:05 -0700 Subject: [PATCH 15/31] "Outer" getBuiltURProgram overload --- .../detail/program_manager/program_manager.cpp | 12 ++++++------ .../detail/program_manager/program_manager.hpp | 8 ++++---- .../program_manager/arg_mask/EliminatedArgMask.cpp | 6 ++++-- 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index e1956a1b84f5f..631cac332e15a 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -840,7 +840,7 @@ static void setSpecializationConstants(device_image_impl &InputImpl, // When caching is enabled, the returned UrProgram will already have // its ref count incremented. -ur_program_handle_t ProgramManager::getBuiltURProgram( +Managed ProgramManager::getBuiltURProgram( context_impl &ContextImpl, device_impl &DeviceImpl, KernelNameStrRefT KernelName, const NDRDescT &NDRDesc) { device_impl *RootDevImpl; @@ -888,8 +888,9 @@ ur_program_handle_t ProgramManager::getBuiltURProgram( std::copy(DeviceImagesToLink.begin(), DeviceImagesToLink.end(), std::back_inserter(AllImages)); - return getBuiltURProgram(std::move(AllImages), ContextImpl, - {RootOrSubDevImpl}); + return Managed{ + getBuiltURProgram(std::move(AllImages), ContextImpl, {RootOrSubDevImpl}), + ContextImpl.getAdapter()}; } ur_program_handle_t @@ -1110,9 +1111,8 @@ FastKernelCacheValPtr ProgramManager::getOrCreateKernel( } } - Managed Program{ - getBuiltURProgram(ContextImpl, DeviceImpl, KernelName, NDRDesc), - ContextImpl.getAdapter()}; + Managed Program = + getBuiltURProgram(ContextImpl, DeviceImpl, KernelName, NDRDesc); auto BuildF = [this, &Program, &KernelName, &ContextImpl] { ur_kernel_handle_t Kernel = nullptr; diff --git a/sycl/source/detail/program_manager/program_manager.hpp b/sycl/source/detail/program_manager/program_manager.hpp index d02cb951024e4..4eb65e4687e08 100644 --- a/sycl/source/detail/program_manager/program_manager.hpp +++ b/sycl/source/detail/program_manager/program_manager.hpp @@ -177,10 +177,10 @@ class ProgramManager { /// \param Context the context to build the program with /// \param Device the device for which the program is built /// \param KernelName the kernel's name - ur_program_handle_t getBuiltURProgram(context_impl &ContextImpl, - device_impl &DeviceImpl, - KernelNameStrRefT KernelName, - const NDRDescT &NDRDesc = {}); + Managed getBuiltURProgram(context_impl &ContextImpl, + device_impl &DeviceImpl, + KernelNameStrRefT KernelName, + const NDRDescT &NDRDesc = {}); /// Builds a program from a given set of images or retrieves that program from /// cache. diff --git a/sycl/unittests/program_manager/arg_mask/EliminatedArgMask.cpp b/sycl/unittests/program_manager/arg_mask/EliminatedArgMask.cpp index f856703f30185..d90c847cd9bfc 100644 --- a/sycl/unittests/program_manager/arg_mask/EliminatedArgMask.cpp +++ b/sycl/unittests/program_manager/arg_mask/EliminatedArgMask.cpp @@ -304,7 +304,8 @@ TEST(EliminatedArgMask, ReuseOfHandleValues) { sycl::queue Queue{Dev}; auto Ctx = Queue.get_context(); ProgBefore = PM.getBuiltURProgram(*sycl::detail::getSyclObjImpl(Ctx), - *sycl::detail::getSyclObjImpl(Dev), Name); + *sycl::detail::getSyclObjImpl(Dev), Name) + .release(); auto Mask = PM.getEliminatedKernelArgMask(ProgBefore, Name); EXPECT_NE(Mask, nullptr); EXPECT_EQ(Mask->at(0), 1); @@ -329,7 +330,8 @@ TEST(EliminatedArgMask, ReuseOfHandleValues) { sycl::queue Queue{Dev}; auto Ctx = Queue.get_context(); ProgAfter = PM.getBuiltURProgram(*sycl::detail::getSyclObjImpl(Ctx), - *sycl::detail::getSyclObjImpl(Dev), Name); + *sycl::detail::getSyclObjImpl(Dev), Name) + .release(); auto Mask = PM.getEliminatedKernelArgMask(ProgAfter, Name); EXPECT_NE(Mask, nullptr); EXPECT_EQ(Mask->at(0), 0); From e2dbbccb11741bf97708d83c664cae552118289c Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Mon, 21 Jul 2025 16:11:49 -0700 Subject: [PATCH 16/31] Remaining ProgramManager::getBuiltURProgram overload --- .../program_manager/program_manager.cpp | 23 ++++++++----------- .../program_manager/program_manager.hpp | 2 +- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index 631cac332e15a..a38db8412dd21 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -888,12 +888,11 @@ Managed ProgramManager::getBuiltURProgram( std::copy(DeviceImagesToLink.begin(), DeviceImagesToLink.end(), std::back_inserter(AllImages)); - return Managed{ - getBuiltURProgram(std::move(AllImages), ContextImpl, {RootOrSubDevImpl}), - ContextImpl.getAdapter()}; + return getBuiltURProgram(std::move(AllImages), ContextImpl, + {RootOrSubDevImpl}); } -ur_program_handle_t +Managed ProgramManager::getBuiltURProgram(const BinImgWithDeps &ImgWithDeps, context_impl &ContextImpl, devices_range Devs, const DevImgPlainWithDeps *DevImgWithDeps, @@ -999,7 +998,7 @@ ProgramManager::getBuiltURProgram(const BinImgWithDeps &ImgWithDeps, }; if (!SYCLConfig::get()) - return BuildF().release(); + return BuildF(); uint32_t ImgId = ImgWithDeps.getMain()->getImageID(); std::set URDevicesSet; @@ -1087,7 +1086,7 @@ ProgramManager::getBuiltURProgram(const BinImgWithDeps &ImgWithDeps, // caller. In that case, we need to increase the ref count of the // program. Adapter.call(ResProgram); - return ResProgram; + return Managed{ResProgram, Adapter}; } FastKernelCacheValPtr ProgramManager::getOrCreateKernel( @@ -3108,7 +3107,7 @@ ProgramManager::build(const DevImgPlainWithDeps &DevImgWithDeps, SpecConstMap = MainInputImpl.get_spec_const_data_ref(); } - ur_program_handle_t ResProgram = getBuiltURProgram( + Managed ResProgram = getBuiltURProgram( std::move(BinImgs), ContextImpl, Devs, &DevImgWithDeps, SpecConstBlob); // The origin becomes the combination of all the origins. @@ -3134,12 +3133,10 @@ ProgramManager::build(const DevImgPlainWithDeps &DevImgWithDeps, DeviceImageImplPtr ExecImpl = device_image_impl::create( ResultBinImg, Context, Devs, bundle_state::executable, - std::move(KernelIDs), - // Move creation of `Managed` up: - Managed{ResProgram, ContextImpl.getAdapter()}, - std::move(SpecConstMap), std::move(SpecConstBlob), CombinedOrigins, - std::move(MergedRTCInfo), std::move(MergedKernelNames), - std::move(MergedEliminatedKernelArgMasks), std::move(MergedImageStorage)); + std::move(KernelIDs), std::move(ResProgram), std::move(SpecConstMap), + std::move(SpecConstBlob), CombinedOrigins, std::move(MergedRTCInfo), + std::move(MergedKernelNames), std::move(MergedEliminatedKernelArgMasks), + std::move(MergedImageStorage)); return createSyclObjFromImpl(std::move(ExecImpl)); } diff --git a/sycl/source/detail/program_manager/program_manager.hpp b/sycl/source/detail/program_manager/program_manager.hpp index 4eb65e4687e08..3eb887c6609dd 100644 --- a/sycl/source/detail/program_manager/program_manager.hpp +++ b/sycl/source/detail/program_manager/program_manager.hpp @@ -192,7 +192,7 @@ class ProgramManager { /// represents the images. /// \param SpecConsts is an optional parameter containing spec constant values /// the program should be built with. - ur_program_handle_t + Managed getBuiltURProgram(const BinImgWithDeps &ImgWithDeps, context_impl &ContextImpl, devices_range Devs, const DevImgPlainWithDeps *DevImgWithDeps = nullptr, From c97ec87c90d694aed8c2899e049204a0aa5ae314 Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Tue, 22 Jul 2025 08:50:08 -0700 Subject: [PATCH 17/31] backend.cpp --- sycl/source/backend.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/sycl/source/backend.cpp b/sycl/source/backend.cpp index 1c6ec1ad3ae5d..5bef2f230e03f 100644 --- a/sycl/source/backend.cpp +++ b/sycl/source/backend.cpp @@ -196,7 +196,7 @@ make_kernel_bundle(ur_native_handle_t NativeHandle, adapter_impl &Adapter = getAdapter(Backend); context_impl &ContextImpl = *getSyclObjImpl(TargetContext); - ur_program_handle_t UrProgram = nullptr; + Managed UrProgram{Adapter}; ur_program_native_properties_t Properties{}; Properties.stype = UR_STRUCTURE_TYPE_PROGRAM_NATIVE_PROPERTIES; Properties.isNativeHandleOwned = !KeepOwnership; @@ -258,7 +258,7 @@ make_kernel_bundle(ur_native_handle_t NativeHandle, "Program and kernel_bundle state mismatch " + detail::codeToString(UR_RESULT_ERROR_INVALID_VALUE)); if (State == bundle_state::executable) { - ur_program_handle_t UrLinkedProgram = nullptr; + Managed UrLinkedProgram{Adapter}; auto Res = Adapter.call_nocheck( ContextImpl.getHandleRef(), 1u, &Dev, 1u, &UrProgram, nullptr, &UrLinkedProgram); @@ -269,7 +269,8 @@ make_kernel_bundle(ur_native_handle_t NativeHandle, } Adapter.checkUrResult(Res); if (UrLinkedProgram != nullptr) { - UrProgram = UrLinkedProgram; + UrProgram.release(); // Isn't that a leak? + UrProgram = std::move(UrLinkedProgram); } } break; @@ -302,9 +303,8 @@ make_kernel_bundle(ur_native_handle_t NativeHandle, // symbols (e.g. when kernel_bundle is supposed to be joined with another). auto KernelIDs = std::make_shared>(); auto DevImgImpl = device_image_impl::create( - nullptr, TargetContext, Devices, State, KernelIDs, - // TODO: Move creation of `Managed` up. - Managed{UrProgram, Adapter}, ImageOriginInterop); + nullptr, TargetContext, Devices, State, KernelIDs, std::move(UrProgram), + ImageOriginInterop); device_image_plain DevImg{DevImgImpl}; return kernel_bundle_impl::create(TargetContext, Devices, DevImg); From 854a46aee48efe31fb9c7ce16492037970c5670f Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Tue, 22 Jul 2025 10:23:27 -0700 Subject: [PATCH 18/31] ProgramBuildResult ctor simplification --- sycl/source/detail/kernel_program_cache.hpp | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/sycl/source/detail/kernel_program_cache.hpp b/sycl/source/detail/kernel_program_cache.hpp index aaf3d8f390ee7..65e0be80aa869 100644 --- a/sycl/source/detail/kernel_program_cache.hpp +++ b/sycl/source/detail/kernel_program_cache.hpp @@ -112,12 +112,11 @@ class KernelProgramCache { }; struct ProgramBuildResult : public BuildResult> { - ProgramBuildResult(adapter_impl &Adapter) { - Val = Managed{Adapter}; - } - ProgramBuildResult(adapter_impl &Adapter, BuildState InitialState) { - Val = Managed{Adapter}; + ProgramBuildResult() = default; + ProgramBuildResult(BuildState InitialState, + Managed &&Prog) { this->State.store(InitialState); + this->Val = std::move(Prog); } #ifdef _MSC_VER #pragma warning(push) @@ -407,7 +406,7 @@ class KernelProgramCache { ProgramCache &ProgCache = LockedCache.get(); auto [It, DidInsert] = ProgCache.Cache.try_emplace(CacheKey, nullptr); if (DidInsert) { - It->second = std::make_shared(getAdapter()); + It->second = std::make_shared(); // Save reference between the common key and the full key. CommonProgramKeyT CommonKey = std::make_pair(CacheKey.first.second, CacheKey.second); @@ -429,9 +428,9 @@ class KernelProgramCache { ProgramCache &ProgCache = LockedCache.get(); auto [It, DidInsert] = ProgCache.Cache.try_emplace(CacheKey, nullptr); if (DidInsert) { - It->second = std::make_shared(getAdapter(), - BuildState::BS_Done); - It->second->Val = Managed{Program, getAdapter()}; + It->second = std::make_shared( + BuildState::BS_Done, + Managed{Program, getAdapter()}); // Save reference between the common key and the full key. CommonProgramKeyT CommonKey = std::make_pair(CacheKey.first.second, CacheKey.second); From 7432a51b532835bccc48dd85b8f318cc5d98f4bc Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Tue, 22 Jul 2025 13:15:18 -0700 Subject: [PATCH 19/31] debug print old --- sycl/source/detail/program_manager/program_manager.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index a38db8412dd21..13d44ee351850 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -1043,6 +1043,7 @@ ProgramManager::getBuiltURProgram(const BinImgWithDeps &ImgWithDeps, // For every cached copy of the program, we need to increment its // refcount Adapter.call(ResProgram); + std::cerr << "Old manual retain " << ResProgram << std::endl; } } }; @@ -1076,6 +1077,7 @@ ProgramManager::getBuiltURProgram(const BinImgWithDeps &ImgWithDeps, // For every cached copy of the program, we need to increment its // refcount Adapter.call(ResProgram); + std::cerr << "Old manual retain2 " << ResProgram << std::endl; } CacheLinkedImages(); } From f0229bf30591eeee8a9d5c5ed22c9662590dfe9c Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Tue, 22 Jul 2025 13:21:29 -0700 Subject: [PATCH 20/31] Proper retain in insertBuiltProgram --- sycl/source/detail/adapter_impl.hpp | 11 +++++++++++ sycl/source/detail/kernel_program_cache.hpp | 10 ++++------ .../detail/program_manager/program_manager.cpp | 15 ++------------- 3 files changed, 17 insertions(+), 19 deletions(-) diff --git a/sycl/source/detail/adapter_impl.hpp b/sycl/source/detail/adapter_impl.hpp index 6aa48f4d43a25..5028acb809d6d 100644 --- a/sycl/source/detail/adapter_impl.hpp +++ b/sycl/source/detail/adapter_impl.hpp @@ -244,6 +244,10 @@ template class Managed { if constexpr (std::is_same_v) return UrApiKind::urProgramRelease; }(); + static constexpr auto Retain = []() constexpr { + if constexpr (std::is_same_v) + return UrApiKind::urProgramRetain; + }(); public: Managed() = default; @@ -292,6 +296,13 @@ template class Managed { Adapter->call(R); } + Managed retain() { + assert(R && "Cannot retain unintialized resource!"); + std::cerr << "Retaining " << R << std::endl; + Adapter->call(R); + return Managed{R, *Adapter}; + } + private: URResource R = nullptr; adapter_impl *Adapter = nullptr; diff --git a/sycl/source/detail/kernel_program_cache.hpp b/sycl/source/detail/kernel_program_cache.hpp index 65e0be80aa869..339efa3b53251 100644 --- a/sycl/source/detail/kernel_program_cache.hpp +++ b/sycl/source/detail/kernel_program_cache.hpp @@ -423,14 +423,13 @@ class KernelProgramCache { // // Returns whether or not an insertion took place. bool insertBuiltProgram(const ProgramCacheKeyT &CacheKey, - ur_program_handle_t Program) { + Managed &Program) { auto LockedCache = acquireCachedPrograms(); ProgramCache &ProgCache = LockedCache.get(); auto [It, DidInsert] = ProgCache.Cache.try_emplace(CacheKey, nullptr); if (DidInsert) { - It->second = std::make_shared( - BuildState::BS_Done, - Managed{Program, getAdapter()}); + It->second = std::make_shared(BuildState::BS_Done, + Program.retain()); // Save reference between the common key and the full key. CommonProgramKeyT CommonKey = std::make_pair(CacheKey.first.second, CacheKey.second); @@ -642,8 +641,7 @@ class KernelProgramCache { // If it is the first time the program is fetched, add it to the eviction // list. void registerProgramFetch(const ProgramCacheKeyT &CacheKey, - const ur_program_handle_t &Program, - const bool IsBuilt) { + ur_program_handle_t Program, const bool IsBuilt) { size_t ProgramCacheEvictionThreshold = SYCLConfig::getProgramCacheSize(); diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index 13d44ee351850..278bd0de49870 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -1022,7 +1022,7 @@ ProgramManager::getBuiltURProgram(const BinImgWithDeps &ImgWithDeps, Cache.getOrBuild(GetCachedBuildF, BuildF, EvictFunc); assert(BuildResult && "getOrBuild isn't supposed to return nullptr!"); - ur_program_handle_t ResProgram = BuildResult->Val; + Managed &ResProgram = BuildResult->Val; // Here we have multiple devices a program is built for, so add the program to // the cache for all subsets of provided list of devices. @@ -1039,12 +1039,6 @@ ProgramManager::getBuiltURProgram(const BinImgWithDeps &ImgWithDeps, bool DidInsert = Cache.insertBuiltProgram(CacheKey, ResProgram); // Add to the eviction list. Cache.registerProgramFetch(CacheKey, ResProgram, DidInsert); - if (DidInsert) { - // For every cached copy of the program, we need to increment its - // refcount - Adapter.call(ResProgram); - std::cerr << "Old manual retain " << ResProgram << std::endl; - } } }; CacheLinkedImages(); @@ -1073,12 +1067,7 @@ ProgramManager::getBuiltURProgram(const BinImgWithDeps &ImgWithDeps, // Change device in the cache key to reduce copying of spec const data. CacheKey.second = std::move(Subset); bool DidInsert = Cache.insertBuiltProgram(CacheKey, ResProgram); - if (DidInsert) { - // For every cached copy of the program, we need to increment its - // refcount - Adapter.call(ResProgram); - std::cerr << "Old manual retain2 " << ResProgram << std::endl; - } + (void)DidInsert; CacheLinkedImages(); } } From 944ac9821f2048c24db1ded9d07e2691c7bb9d01 Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Tue, 22 Jul 2025 13:28:00 -0700 Subject: [PATCH 21/31] One more retain --- .../detail/program_manager/program_manager.cpp | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index 278bd0de49870..4167b8afcc2eb 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -1072,12 +1072,15 @@ ProgramManager::getBuiltURProgram(const BinImgWithDeps &ImgWithDeps, } } - // If caching is enabled, one copy of the program handle will be - // stored in the cache, and one handle is returned to the - // caller. In that case, we need to increase the ref count of the - // program. - Adapter.call(ResProgram); - return Managed{ResProgram, Adapter}; + // We don't know if `BuildResult` above is a single owner of this program (no + // caching) or not (shared ownership with the record in the cache), so we + // can't just `std::move(ResProgram)` that references + // `Managed` inside `BuildResult` and have to `retain`. + // + // If this a single owner indeed, then `BuildResult` will be automatically + // destructed upon return and would cause automatic `urProgramRelease` which + // might be unoptimal but still correct. + return ResProgram.retain(); } FastKernelCacheValPtr ProgramManager::getOrCreateKernel( From 11e0a3638506477782cd94e1c8a34a01537148cd Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Tue, 22 Jul 2025 13:38:33 -0700 Subject: [PATCH 22/31] Remove unused `Adapter` after managed retains --- sycl/source/detail/program_manager/program_manager.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index 4167b8afcc2eb..592d9aed249f0 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -1026,11 +1026,10 @@ ProgramManager::getBuiltURProgram(const BinImgWithDeps &ImgWithDeps, // Here we have multiple devices a program is built for, so add the program to // the cache for all subsets of provided list of devices. - adapter_impl &Adapter = ContextImpl.getAdapter(); + // If we linked any extra device images, then we need to // cache them as well. - auto CacheLinkedImages = [&Adapter, &Cache, &CacheKey, &ResProgram, - &ImgWithDeps] { + auto CacheLinkedImages = [&Cache, &CacheKey, &ResProgram, &ImgWithDeps] { for (auto It = ImgWithDeps.depsBegin(); It != ImgWithDeps.depsEnd(); ++It) { const RTDeviceBinaryImage *BImg = *It; // CacheKey is captured by reference by GetCachedBuildF, so we can simply From f5221d9df36d4e561fc85f25ad38f45ce1f85ea2 Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Tue, 22 Jul 2025 15:13:17 -0700 Subject: [PATCH 23/31] loadDeviceLibFallback (last retain) --- sycl/source/detail/adapter_impl.hpp | 6 ++ .../program_manager/program_manager.cpp | 74 +++++++++---------- 2 files changed, 39 insertions(+), 41 deletions(-) diff --git a/sycl/source/detail/adapter_impl.hpp b/sycl/source/detail/adapter_impl.hpp index 5028acb809d6d..c55b1ccfdca76 100644 --- a/sycl/source/detail/adapter_impl.hpp +++ b/sycl/source/detail/adapter_impl.hpp @@ -303,6 +303,12 @@ template class Managed { return Managed{R, *Adapter}; } + bool operator==(const Managed &Other) const { + assert((!Adapter || !Other.Adapter || Adapter == Other.Adapter) && + "Objects must belong to the same adapter!"); + return R == Other.R; + } + private: URResource R = nullptr; adapter_impl *Adapter = nullptr; diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index 592d9aed249f0..584c05675b51f 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -1327,82 +1327,74 @@ loadDeviceLibFallback(context_impl &Context, DeviceLibExt Extension, // compiled for a device if there is a corresponding record in the per-context // cache. std::vector DevicesToCompile; - ur_program_handle_t URProgram = nullptr; + Managed *UrProgram = nullptr; assert(Devices.size() > 0 && "At least one device is expected in the input vector"); // Vector of devices that don't have the library cached. - for (auto Dev : Devices) { - auto CacheResult = CachedLibPrograms.emplace(std::make_pair( - std::make_pair(Extension, Dev), Managed{})); - bool Cached = !CacheResult.second; - if (!Cached) { - DevicesToCompile.push_back(Dev); - } else { - ur_program_handle_t CachedURProgram = CacheResult.first->second; - assert(CachedURProgram && "If device lib UR program was cached then is " + for (ur_device_handle_t Dev : Devices) { + auto [It, Inserted] = CachedLibPrograms.emplace( + std::make_pair(Extension, Dev), Managed{}); + if (!Inserted) { + Managed &CachedUrProgram = It->second; + assert(CachedUrProgram && "If device lib UR program was cached then is " "expected to be not a nullptr"); - assert(((URProgram && URProgram == CachedURProgram) || (!URProgram)) && - "All cached UR programs should be the same"); - if (!URProgram) - URProgram = CachedURProgram; + assert(!UrProgram || *UrProgram == CachedUrProgram); + // Managed::operator& is overloaded, use + // `std::addressof`: + UrProgram = std::addressof(CachedUrProgram); + } else { + DevicesToCompile.push_back(Dev); } } + if (DevicesToCompile.empty()) - return URProgram; + return *UrProgram; auto EraseProgramForDevices = [&]() { for (auto Dev : DevicesToCompile) CachedLibPrograms.erase(std::make_pair(Extension, Dev)); }; - bool IsProgramCreated = !URProgram; + Managed NewlyCreated; // Create UR program for device lib if we don't have it yet. - if (!URProgram) { - Managed DeviceLibProgram = - loadDeviceLib(Context, LibFileName); - if (DeviceLibProgram == nullptr) { + if (!UrProgram) { + NewlyCreated = loadDeviceLib(Context, LibFileName); + if (NewlyCreated == nullptr) { EraseProgramForDevices(); throw exception(make_error_code(errc::build), std::string("Failed to load ") + LibFileName); } + } - // TODO: How isn't this a leak? - URProgram = DeviceLibProgram.release(); + // Insert UrProgram into the cache for all devices that we will compile for. + for (auto Dev : DevicesToCompile) { + Managed &Cached = + CachedLibPrograms[std::make_pair(Extension, Dev)]; + if (NewlyCreated) { + Cached = std::move(NewlyCreated); + UrProgram = std::addressof(Cached); + } else { + Cached = UrProgram->retain(); + } } - // Insert URProgram into the cache for all devices that we compiled it for. - // Retain UR program for each record in the cache. adapter_impl &Adapter = Context.getAdapter(); - - // UR program handle is stored in the cache for each device that we compiled - // it for. We have to retain UR program for each record in the cache. We need - // to take into account that UR program creation makes its reference count to - // be 1. - size_t RetainCount = - IsProgramCreated ? DevicesToCompile.size() - 1 : DevicesToCompile.size(); - for (size_t I = 0; I < RetainCount; ++I) - Adapter.call(URProgram); - - for (auto Dev : DevicesToCompile) - CachedLibPrograms[std::make_pair(Extension, Dev)] = - Managed{URProgram, Adapter}; - // TODO no spec constants are used in the std libraries, support in the future // Do not use compile options for library programs: it is not clear if user // options (image options) are supposed to be applied to library program as // well, and what actually happens to a SPIR-V program if we apply them. ur_result_t Error = - doCompile(Adapter, URProgram, DevicesToCompile.size(), + doCompile(Adapter, *UrProgram, DevicesToCompile.size(), DevicesToCompile.data(), Context.getHandleRef(), ""); if (Error != UR_RESULT_SUCCESS) { EraseProgramForDevices(); throw detail::set_ur_error( exception(make_error_code(errc::build), - ProgramManager::getProgramBuildLog(URProgram, Context)), + ProgramManager::getProgramBuildLog(*UrProgram, Context)), Error); } - return URProgram; + return *UrProgram; } ProgramManager::ProgramManager() From 4f06a1a94f69608a37b13ef6cf94bf265aa4c41e Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Tue, 22 Jul 2025 15:30:14 -0700 Subject: [PATCH 24/31] Drop debug printing --- sycl/source/detail/adapter_impl.hpp | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/sycl/source/detail/adapter_impl.hpp b/sycl/source/detail/adapter_impl.hpp index c55b1ccfdca76..08f1f44947ff8 100644 --- a/sycl/source/detail/adapter_impl.hpp +++ b/sycl/source/detail/adapter_impl.hpp @@ -251,9 +251,7 @@ template class Managed { public: Managed() = default; - Managed(URResource R, adapter_impl &Adapter) : R(R), Adapter(&Adapter) { - std::cerr << "Ctor created: " << R << std::endl; - } + Managed(URResource R, adapter_impl &Adapter) : R(R), Adapter(&Adapter) {} Managed(adapter_impl &Adapter) : Adapter(&Adapter) {} Managed(const Managed &) = delete; Managed(Managed &&Other) : Adapter(Other.Adapter) { @@ -262,10 +260,9 @@ template class Managed { } Managed &operator=(const Managed &) = delete; Managed &operator=(Managed &&Other) { - if (R) { - std::cerr << "Assign releasing: " << R << std::endl; + if (R) Adapter->call(R); - } + R = Other.R; Other.R = nullptr; Adapter = Other.Adapter; @@ -275,7 +272,6 @@ template class Managed { operator URResource() const { return R; } URResource release() { - std::cerr << "Manually releasing: " << R << std::endl; URResource Res = R; R = nullptr; return Res; @@ -284,7 +280,6 @@ template class Managed { URResource *operator&() { assert(!R && "Already initialized!"); assert(Adapter && "Adapter must be set for this API!"); - std::cerr << "Creating externally..." << std::endl; return &R; } @@ -292,13 +287,11 @@ template class Managed { if (!R) return; - std::cerr << "Dtor releasing: " << R << std::endl; Adapter->call(R); } Managed retain() { assert(R && "Cannot retain unintialized resource!"); - std::cerr << "Retaining " << R << std::endl; Adapter->call(R); return Managed{R, *Adapter}; } From f59f3c6f4ac2ee2d0de54a8d9ef71e2de20295c2 Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Tue, 22 Jul 2025 15:30:51 -0700 Subject: [PATCH 25/31] clang-format --- sycl/source/detail/device_image_impl.hpp | 6 +++--- sycl/source/detail/program_manager/program_manager.cpp | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/sycl/source/detail/device_image_impl.hpp b/sycl/source/detail/device_image_impl.hpp index d050d9b76bf66..d028f6b9afa81 100644 --- a/sycl/source/detail/device_image_impl.hpp +++ b/sycl/source/detail/device_image_impl.hpp @@ -257,11 +257,11 @@ class device_image_impl device_image_impl(const RTDeviceBinaryImage *BinImage, context Context, devices_range Devices, bundle_state State, std::shared_ptr> KernelIDs, - Managed &&Program, uint8_t Origins, private_tag) + Managed &&Program, uint8_t Origins, + private_tag) : MBinImage(BinImage), MContext(std::move(Context)), MDevices(Devices.to>()), MState(State), - MProgram(std::move(Program)), - MKernelIDs(std::move(KernelIDs)), + MProgram(std::move(Program)), MKernelIDs(std::move(KernelIDs)), MSpecConstsDefValBlob(getSpecConstsDefValBlob()), MOrigins(Origins) { updateSpecConstSymMap(); if (BinImage && (MOrigins & ImageOriginSYCLBIN)) { diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index 584c05675b51f..8410a85634ab4 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -92,9 +92,9 @@ createBinaryProgram(context_impl &Context, devices_range Devices, return Program; } -static Managed createSpirvProgram(context_impl &Context, - const unsigned char *Data, - size_t DataLen) { +static Managed +createSpirvProgram(context_impl &Context, const unsigned char *Data, + size_t DataLen) { adapter_impl &Adapter = Context.getAdapter(); Managed Program{Adapter}; Adapter.call(Context.getHandleRef(), Data, From 28d0048497db8b661c4cf9e47a325304627867f0 Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Tue, 22 Jul 2025 15:47:00 -0700 Subject: [PATCH 26/31] rvalue-ref in device_image_impl ctor --- sycl/source/detail/device_image_impl.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sycl/source/detail/device_image_impl.hpp b/sycl/source/detail/device_image_impl.hpp index d028f6b9afa81..97daad803229d 100644 --- a/sycl/source/detail/device_image_impl.hpp +++ b/sycl/source/detail/device_image_impl.hpp @@ -349,7 +349,7 @@ class device_image_impl } device_image_impl(const context &Context, devices_range Devices, - bundle_state State, Managed Program, + bundle_state State, Managed &&Program, syclex::source_language Lang, KernelNameSetT &&KernelNames, private_tag) : MBinImage(static_cast(nullptr)), From 5208f8676ce9c9fbc527aca0a885ab79eeb89515 Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Tue, 22 Jul 2025 15:50:34 -0700 Subject: [PATCH 27/31] Remove stale comment --- sycl/source/detail/kernel_program_cache.hpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/sycl/source/detail/kernel_program_cache.hpp b/sycl/source/detail/kernel_program_cache.hpp index 339efa3b53251..d1745819954d0 100644 --- a/sycl/source/detail/kernel_program_cache.hpp +++ b/sycl/source/detail/kernel_program_cache.hpp @@ -796,9 +796,6 @@ class KernelProgramCache { // only the building thread will run this try { - // Remove `adapter_impl` from `ProgramBuildResult`'s ctors once `Build` - // returns `ManagedVal)>, "Are we casting from Managed to plain URResource?"); From 49dc0bacb4f78bf44487bffefb8ce21dcb4374ee Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Wed, 23 Jul 2025 07:36:22 -0700 Subject: [PATCH 28/31] Fix for the failing e2e test --- sycl/source/backend.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sycl/source/backend.cpp b/sycl/source/backend.cpp index 5bef2f230e03f..d00220d3341de 100644 --- a/sycl/source/backend.cpp +++ b/sycl/source/backend.cpp @@ -259,12 +259,13 @@ make_kernel_bundle(ur_native_handle_t NativeHandle, detail::codeToString(UR_RESULT_ERROR_INVALID_VALUE)); if (State == bundle_state::executable) { Managed UrLinkedProgram{Adapter}; + ur_program_handle_t ProgramsToLink[] = {UrProgram}; auto Res = Adapter.call_nocheck( - ContextImpl.getHandleRef(), 1u, &Dev, 1u, &UrProgram, nullptr, + ContextImpl.getHandleRef(), 1u, &Dev, 1u, ProgramsToLink, nullptr, &UrLinkedProgram); if (Res == UR_RESULT_ERROR_UNSUPPORTED_FEATURE) { Res = Adapter.call_nocheck( - ContextImpl.getHandleRef(), 1u, &UrProgram, nullptr, + ContextImpl.getHandleRef(), 1u, ProgramsToLink, nullptr, &UrLinkedProgram); } Adapter.checkUrResult(Res); From fff3362e4699f6159d8b600f8f0488659dcd97dc Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Wed, 23 Jul 2025 07:42:04 -0700 Subject: [PATCH 29/31] Remove stale comment - context_impl owns these --- sycl/source/detail/program_manager/program_manager.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index 8410a85634ab4..54d24808475e3 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -1752,8 +1752,6 @@ Managed ProgramManager::build( Context.getHandleRef(), CompileOptions.c_str()); Adapter.checkUrResult(Res); } - // Should be `std::move(Program)` once `LinkPrograms` is switched to - // `Managed Date: Wed, 23 Jul 2025 13:33:37 -0700 Subject: [PATCH 30/31] Fix one leak --- sycl/source/backend.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/sycl/source/backend.cpp b/sycl/source/backend.cpp index d00220d3341de..7c8fc1505c534 100644 --- a/sycl/source/backend.cpp +++ b/sycl/source/backend.cpp @@ -270,7 +270,6 @@ make_kernel_bundle(ur_native_handle_t NativeHandle, } Adapter.checkUrResult(Res); if (UrLinkedProgram != nullptr) { - UrProgram.release(); // Isn't that a leak? UrProgram = std::move(UrLinkedProgram); } } From 8fcca38915ea0e7b2761e7ef8ca8984722cb5b56 Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Wed, 23 Jul 2025 13:35:38 -0700 Subject: [PATCH 31/31] Update test --- sycl/test-e2e/Basic/interop/construction_ocl.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sycl/test-e2e/Basic/interop/construction_ocl.cpp b/sycl/test-e2e/Basic/interop/construction_ocl.cpp index 19dd22db217e3..adbc0e0832874 100644 --- a/sycl/test-e2e/Basic/interop/construction_ocl.cpp +++ b/sycl/test-e2e/Basic/interop/construction_ocl.cpp @@ -14,22 +14,22 @@ // CHECK: <--- urProgramRelease(.hProgram = [[PROG1]]) -> UR_RESULT_SUCCESS // CHECK: <--- urProgramCreateWithNativeHandle{{.*}} .phProgram = {{.*}} ([[PROG4:.*]])) -> UR_RESULT_SUCCESS // CHECK: <--- urProgramCreateWithNativeHandle{{.*}} .phProgram = {{.*}} ([[PROG5:.*]])) -> UR_RESULT_SUCCESS +// CHECK: <--- urProgramRelease(.hProgram = [[PROG5]]) -> UR_RESULT_SUCCESS // CHECK: <--- urProgramCreateWithNativeHandle{{.*}} .phProgram = {{.*}} ([[PROG6:.*]])) -> UR_RESULT_SUCCESS // CHECK: <--- urProgramLinkExp{{.*}} -> UR_RESULT_ERROR_UNSUPPORTED_FEATURE // CHECK: <--- urProgramLink{{.*}} .phProgram = {{.*}} ([[PROG7:.*]])) -> UR_RESULT_SUCCESS +// CHECK: <--- urProgramRelease(.hProgram = [[PROG6]]) -> UR_RESULT_SUCCESS // CHECK: <--- urProgramRelease(.hProgram = [[PROG7]]) -> UR_RESULT_SUCCESS // CHECK: <--- urProgramRelease(.hProgram = [[PROG4]]) -> UR_RESULT_SUCCESS // CHECK: <--- urProgramCreateWithNativeHandle{{.*}}.phProgram = {{.*}} ([[PROG8:.*]])) -> UR_RESULT_SUCCESS // CHECK: <--- urProgramCreateWithNativeHandle{{.*}}.phProgram = {{.*}} ([[PROG9:.*]])) -> UR_RESULT_SUCCESS +// CHECK: <--- urProgramRelease(.hProgram = [[PROG9]]) -> UR_RESULT_SUCCESS // CHECK: <--- urProgramCreateWithNativeHandle{{.*}}.phProgram = {{.*}} ([[PROG10:.*]])) -> UR_RESULT_SUCCESS +// CHECK: <--- urProgramRelease(.hProgram = [[PROG10]]) -> UR_RESULT_SUCCESS // CHECK: <--- urProgramRelease(.hProgram = [[PROG8]]) -> UR_RESULT_SUCCESS // CHECK: <--- urProgramRelease(.hProgram = [[PROG0]]) -> UR_RESULT_SUCCESS // CHECK: <--- urProgramRelease(.hProgram = [[PROG0]]) -> UR_RESULT_SUCCESS -// 21 lines total, 8 releases, 1 unsuccessful -// 12 create/link/retain, 8 release -// Leaked: PROG5, PROG6, PROG9, PROG10 - #include #include #include