Address reviews.

Author: uditagarwal97

sycl/doc/EnvironmentVariables.md

@@ -14,7 +14,7 @@ compiler and runtime.
 | `SYCL_CACHE_DISABLE_PERSISTENT (deprecated)` | Any(\*) | Has no effect. |
 | `SYCL_CACHE_PERSISTENT` | Integer | Controls persistent device compiled code cache. Turns it on if set to '1' and turns it off if set to '0'. When cache is enabled SYCL runtime will try to cache and reuse JIT-compiled binaries. Default is off. |
 | `SYCL_CACHE_IN_MEM` | '1' or '0' | Enable ('1') or disable ('0') in-memory caching of device compiled code. When cache is enabled SYCL runtime will try to cache and reuse JIT-compiled binaries. Default is '1'. |
-| `SYCL_IN_MEM_CACHE_EVICTION_THRESHOLD` | Positive integer  | `SYCL_IN_MEM_CACHE_EVICTION_THRESHOLD` accepts an integer that specifies the maximum size of the in-memory Program cache in bytes. Eviction is performed when the cache size exceeds the threshold. The default value is 0 which means that eviction is disabled.  |
+| `SYCL_IN_MEM_CACHE_EVICTION_THRESHOLD` | Positive integer  | `SYCL_IN_MEM_CACHE_EVICTION_THRESHOLD` accepts an integer that specifies the maximum size of the in-memory program cache in bytes. Eviction is performed when the cache size exceeds the threshold. The default value is 0 which means that eviction is disabled.  |
 | `SYCL_CACHE_EVICTION_DISABLE` | Any(\*) | Switches persistent cache eviction off when the variable is set. |
 | `SYCL_CACHE_MAX_SIZE` | Positive integer | Persistent cache eviction is triggered once total size of cached images exceeds the value in megabytes (default - 8 192 for 8 GB). Set to 0 to disable size-based cache eviction. |
 | `SYCL_CACHE_THRESHOLD` | Positive integer | Persistent cache eviction threshold in days (default value is 7 for 1 week). Set to 0 for disabling time-based cache eviction. |

sycl/source/detail/kernel_program_cache.hpp

@@ -233,7 +233,8 @@ class KernelProgramCache {
       ::boost::unordered_flat_map<KernelFastCacheKeyT, KernelFastCacheValT>;
 
   // DS to hold data and functions related to Program cache eviction.
-  struct EvictionListT {
+  struct EvictionList {
+  private:
     // Linked list of cache entries to be evicted in case of cache overflow.
     std::list<ProgramCacheKeyT> MProgramEvictionList;
 
@@ -242,6 +243,11 @@ class KernelProgramCache {
                        ProgramCacheKeyHash, ProgramCacheKeyEqual>
         MProgramToEvictionListMap;
 
+  public:
+    std::list<ProgramCacheKeyT> &getProgramEvictionList() {
+      return MProgramEvictionList;
+    }
+
     void clear() {
       MProgramEvictionList.clear();
       MProgramToEvictionListMap.clear();
@@ -342,7 +348,7 @@ class KernelProgramCache {
     return {MKernelsPerProgramCache, MKernelsPerProgramCacheMutex};
   }
 
-  Locked<EvictionListT> acquireEvictionList() {
+  Locked<EvictionList> acquireEvictionList() {
     return {MEvictionList, MProgramEvictionListMutex};
   }
 
@@ -447,12 +453,13 @@ class KernelProgramCache {
     // Evict programs from the beginning of the cache.
     {
       std::lock_guard<std::mutex> Lock(MProgramEvictionListMutex);
-
+      auto &ProgramEvictionList = MEvictionList.getProgramEvictionList();
       size_t CurrCacheSize = MCachedPrograms.ProgramCacheSizeInBytes;
+
       // Traverse the eviction list and remove the LRU programs.
       // The LRU programs will be at the front of the list.
       while (CurrCacheSize > DesiredCacheSize && !MEvictionList.empty()) {
-        ProgramCacheKeyT CacheKey = MEvictionList.MProgramEvictionList.front();
+        ProgramCacheKeyT CacheKey = ProgramEvictionList.front();
         auto LockedCache = acquireCachedPrograms();
         auto &ProgCache = LockedCache.get();
         auto It = ProgCache.Cache.find(CacheKey);
@@ -551,7 +558,7 @@ class KernelProgramCache {
 
       // Store size of the program and check if we need to evict some entries.
       // Get Size of the program.
-      size_t ProgramSize;
+      size_t ProgramSize = 0;
       auto Adapter = getAdapter();
 
       try {
@@ -655,7 +662,7 @@ class KernelProgramCache {
         // Build succeeded.
         if (NewState == BuildState::BS_Done) {
           if constexpr (!std::is_same_v<EvictFT, void *>)
-            EvictFunc(BuildResult->Val, 0);
+            EvictFunc(BuildResult->Val, /*IsBuilt=*/false);
           return BuildResult;
         }
 
@@ -682,7 +689,7 @@ class KernelProgramCache {
         BuildResult->Val = Build();
 
         if constexpr (!std::is_same_v<EvictFT, void *>)
-          EvictFunc(BuildResult->Val, 1);
+          EvictFunc(BuildResult->Val, /*IsBuilt=*/true);
 
         BuildResult->updateAndNotify(BuildState::BS_Done);
         return BuildResult;
@@ -723,7 +730,7 @@ class KernelProgramCache {
   std::unordered_map<ur_program_handle_t, std::vector<KernelFastCacheKeyT>>
       MProgramToKernelFastCacheKeyMap;
 
-  EvictionListT MEvictionList;
+  EvictionList MEvictionList;
   // Mutexes that will be used when accessing the eviction lists.
   std::mutex MProgramEvictionListMutex;
 

sycl/source/detail/program_manager/program_manager.cpp

@@ -921,7 +921,7 @@ ur_program_handle_t ProgramManager::getBuiltURProgram(
     return BuildF();
 
   auto EvictFunc = [&Cache, &CacheKey](ur_program_handle_t Program,
-                                       int isBuilt) {
+                                       bool isBuilt) {
     return Cache.registerProgramFetch(CacheKey, Program, isBuilt);
   };
 
@@ -940,13 +940,12 @@ ur_program_handle_t ProgramManager::getBuiltURProgram(
     // update it here and re-use that lambda.
     CacheKey.first.second = BImg->getImageID();
     bool DidInsert = Cache.insertBuiltProgram(CacheKey, ResProgram);
-    if (DidInsert) {
-      // Add to the eviction list.
-      Cache.registerProgramFetch(CacheKey, ResProgram, 1);
+
+    // 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<UrApiKind::urProgramRetain>(ResProgram);
-    } else
-      Cache.registerProgramFetch(CacheKey, ResProgram, 0);
   }
 
   // If caching is enabled, one copy of the program handle will be
@@ -2742,7 +2741,7 @@ device_image_plain ProgramManager::build(const device_image_plain &DeviceImage,
   };
 
   auto EvictFunc = [&Cache, &CacheKey](ur_program_handle_t Program,
-                                       int isBuilt) {
+                                       bool isBuilt) {
     return Cache.registerProgramFetch(CacheKey, Program, isBuilt);
   };
 

sycl/unittests/assert/assert.cpp

@@ -320,9 +320,7 @@ static ur_result_t redefinedProgramGetInfo(void *pParams) {
   }
 
   // Required if program cache eviction is enabled.
-  if (UR_PROGRAM_INFO_BINARY_SIZES == *params.ppropName ||
-      UR_PROGRAM_INFO_NUM_DEVICES == *params.ppropName) {
-
+  if (UR_PROGRAM_INFO_BINARY_SIZES == *params.ppropName) {
     size_t BinarySize = 1;
 
     if (*params.ppPropValue)