Fix: svm pointer caching

ddabek-i · Compute-Runtime-Automation · commit d0a32b14371c · 2022-04-25T07:59:47.000+02:00
Given a following sequence of setting kernel arg svm pointers: A, nullptr, A
the kernel arg was not being set to A

Related-To: NEO-6895

Signed-off-by: Dominik Dabek &lt;dominik.dabek@intel.com&gt;
diff --git a/level_zero/core/source/kernel/kernel_imp.cpp b/level_zero/core/source/kernel/kernel_imp.cpp
@@ -557,9 +557,9 @@ ze_result_t KernelImp::setArgBuffer(uint32_t argIndex, size_t argSize, const voi
     const auto driverHandle = static_cast<DriverHandleImp *>(device->getDriverHandle());
     const auto svmAllocsManager = driverHandle->getSvmAllocsManager();
     const auto allocationsCounter = svmAllocsManager->allocationsCounter.load();
+    const auto &argInfo = this->kernelArgInfos[argIndex];
     NEO::SvmAllocationData *allocData = nullptr;
     if (argVal != nullptr) {
-        const auto &argInfo = this->kernelArgInfos[argIndex];
         const auto requestedAddress = *reinterpret_cast<void *const *>(argVal);
         if (argInfo.allocId > 0 && requestedAddress == argInfo.value) {
             bool reuseFromCache = false;
@@ -578,12 +578,17 @@ ze_result_t KernelImp::setArgBuffer(uint32_t argIndex, size_t argSize, const voi
                 }
             }
         }
+    } else {
+        if (argInfo.isSetToNullptr) {
+            return ZE_RESULT_SUCCESS;
+        }
     }
 
     const auto &allArgs = kernelImmData->getDescriptor().payloadMappings.explicitArgs;
     const auto &currArg = allArgs[argIndex];
     if (currArg.getTraits().getAddressQualifier() == NEO::KernelArgMetadata::AddrLocal) {
         slmArgSizes[argIndex] = static_cast<uint32_t>(argSize);
+        kernelArgInfos[argIndex] = KernelArgInfo{nullptr, 0, 0, false};
         UNRECOVERABLE_IF(NEO::isUndefinedOffset(currArg.as<NEO::ArgDescPointer>().slmOffset));
         auto slmOffset = *reinterpret_cast<uint32_t *>(crossThreadData.get() + currArg.as<NEO::ArgDescPointer>().slmOffset);
         slmOffset += static_cast<uint32_t>(argSize);
@@ -610,6 +615,7 @@ ze_result_t KernelImp::setArgBuffer(uint32_t argIndex, size_t argSize, const voi
         const auto &arg = kernelImmData->getDescriptor().payloadMappings.explicitArgs[argIndex].as<NEO::ArgDescPointer>();
         uintptr_t nullBufferValue = 0;
         NEO::patchPointer(ArrayRef<uint8_t>(crossThreadData.get(), crossThreadDataSize), arg, nullBufferValue);
+        kernelArgInfos[argIndex] = KernelArgInfo{nullptr, 0, 0, true};
         return ZE_RESULT_SUCCESS;
     }
     const auto requestedAddress = *reinterpret_cast<void *const *>(argVal);
@@ -637,7 +643,7 @@ ze_result_t KernelImp::setArgBuffer(uint32_t argIndex, size_t argSize, const voi
     }
 
     const uint32_t allocId = allocData ? allocData->getAllocId() : 0u;
-    kernelArgInfos[argIndex] = KernelArgInfo{requestedAddress, allocId, allocationsCounter};
+    kernelArgInfos[argIndex] = KernelArgInfo{requestedAddress, allocId, allocationsCounter, false};
 
     return setArgBufferWithAlloc(argIndex, gpuAddress, alloc);
 }
diff --git a/level_zero/core/source/kernel/kernel_imp.h b/level_zero/core/source/kernel/kernel_imp.h
@@ -21,6 +21,7 @@ struct KernelArgInfo {
     const void *value;
     uint32_t allocId;
     uint32_t allocIdMemoryManagerCounter;
+    bool isSetToNullptr = false;
 };
 
 struct KernelImp : Kernel {
diff --git a/level_zero/core/test/unit_tests/sources/kernel/test_kernel.cpp b/level_zero/core/test/unit_tests/sources/kernel/test_kernel.cpp
@@ -147,7 +147,23 @@ TEST_F(SetKernelArgCacheTest, givenValidBufferArgumentWhenSetMultipleTimesThenSe
     EXPECT_EQ(ZE_RESULT_SUCCESS, mockKernel.setArgBuffer(0, sizeof(secondSvmAllocation), &secondSvmAllocation));
     EXPECT_EQ(++callCounter, mockKernel.setArgBufferWithAllocCalled);
 
-    //same value but no svmData - ZE_RESULT_ERROR_INVALID_ARGUMENT
+    //nullptr - not called, argInfo is updated
+    EXPECT_FALSE(mockKernel.kernelArgInfos[0].isSetToNullptr);
+    EXPECT_EQ(ZE_RESULT_SUCCESS, mockKernel.setArgBuffer(0, sizeof(nullptr), nullptr));
+    EXPECT_EQ(callCounter, mockKernel.setArgBufferWithAllocCalled);
+    EXPECT_TRUE(mockKernel.kernelArgInfos[0].isSetToNullptr);
+
+    //nullptr again - not called
+    EXPECT_EQ(ZE_RESULT_SUCCESS, mockKernel.setArgBuffer(0, sizeof(nullptr), nullptr));
+    EXPECT_EQ(callCounter, mockKernel.setArgBufferWithAllocCalled);
+    EXPECT_TRUE(mockKernel.kernelArgInfos[0].isSetToNullptr);
+
+    //same value as before nullptr - called, argInfo is updated
+    EXPECT_EQ(ZE_RESULT_SUCCESS, mockKernel.setArgBuffer(0, sizeof(secondSvmAllocation), &secondSvmAllocation));
+    EXPECT_EQ(++callCounter, mockKernel.setArgBufferWithAllocCalled);
+    EXPECT_FALSE(mockKernel.kernelArgInfos[0].isSetToNullptr);
+
+    // same value but no svmData - ZE_RESULT_ERROR_INVALID_ARGUMENT
     svmAllocsManager->freeSVMAlloc(secondSvmAllocation);
     ++svmAllocsManager->allocationsCounter;
     EXPECT_EQ(ZE_RESULT_ERROR_INVALID_ARGUMENT, mockKernel.setArgBuffer(0, sizeof(secondSvmAllocation), &secondSvmAllocation));
diff --git a/opencl/source/api/api.cpp b/opencl/source/api/api.cpp
@@ -4900,6 +4900,11 @@ cl_int CL_API_CALL clSetKernelArgSVMPointer(cl_kernel kernel,
                 }
             }
         }
+    } else {
+        if (pMultiDeviceKernel->getKernelArguments()[argIndex].isSetToNullptr) {
+            TRACING_EXIT(clSetKernelArgSVMPointer, &retVal);
+            return CL_SUCCESS;
+        }
     }
 
     DBG_LOG_INPUTS("kernel", kernel, "argIndex", argIndex, "argValue", argValue);
diff --git a/opencl/source/kernel/kernel.cpp b/opencl/source/kernel/kernel.cpp
@@ -931,6 +931,7 @@ cl_int Kernel::setArgSvmAlloc(uint32_t argIndex, void *svmPtr, GraphicsAllocatio
     storeKernelArg(argIndex, SVM_ALLOC_OBJ, svmAlloc, svmPtr, sizeof(uintptr_t));
     kernelArguments[argIndex].allocId = allocId;
     kernelArguments[argIndex].allocIdMemoryManagerCounter = allocId ? this->getContext().getSVMAllocsManager()->allocationsCounter.load() : 0u;
+    kernelArguments[argIndex].isSetToNullptr = nullptr == svmPtr;
     if (!kernelArguments[argIndex].isPatched) {
         patchedArgumentsNum++;
         kernelArguments[argIndex].isPatched = true;
diff --git a/opencl/source/kernel/kernel.h b/opencl/source/kernel/kernel.h
@@ -69,6 +69,7 @@ class Kernel : public ReferenceTrackedObject<Kernel> {
         bool isStatelessUncacheable = false;
         uint32_t allocId;
         uint32_t allocIdMemoryManagerCounter;
+        bool isSetToNullptr = false;
     };
 
     enum class TunningStatus {
diff --git a/opencl/test/unit_test/api/cl_set_kernel_arg_svm_pointer_tests.inl b/opencl/test/unit_test/api/cl_set_kernel_arg_svm_pointer_tests.inl
@@ -298,6 +298,39 @@ TEST_F(clSetKernelArgSVMPointerTests, GivenSvmAndValidArgValueWhenSettingSameKer
         EXPECT_EQ(callCounter, pMockKernel->setArgSvmAllocCalls);
         ++mockSvmManager->allocationsCounter;
 
+        // nullptr - called
+        EXPECT_FALSE(pMockKernel->getKernelArguments()[0].isSetToNullptr);
+        retVal = clSetKernelArgSVMPointer(
+            pMockMultiDeviceKernel, // cl_kernel kernel
+            0,                      // cl_uint arg_index
+            nullptr                 // const void *arg_value
+        );
+        EXPECT_EQ(CL_SUCCESS, retVal);
+        EXPECT_EQ(++callCounter, pMockKernel->setArgSvmAllocCalls);
+        EXPECT_TRUE(pMockKernel->getKernelArguments()[0].isSetToNullptr);
+        ++mockSvmManager->allocationsCounter;
+
+        // nullptr again - not called
+        retVal = clSetKernelArgSVMPointer(
+            pMockMultiDeviceKernel, // cl_kernel kernel
+            0,                      // cl_uint arg_index
+            nullptr                 // const void *arg_value
+        );
+        EXPECT_EQ(CL_SUCCESS, retVal);
+        EXPECT_EQ(callCounter, pMockKernel->setArgSvmAllocCalls);
+        EXPECT_TRUE(pMockKernel->getKernelArguments()[0].isSetToNullptr);
+        ++mockSvmManager->allocationsCounter;
+
+        // same value as before nullptr - called
+        retVal = clSetKernelArgSVMPointer(
+            pMockMultiDeviceKernel, // cl_kernel kernel
+            0,                      // cl_uint arg_index
+            nextPtrSvm              // const void *arg_value
+        );
+        EXPECT_EQ(CL_SUCCESS, retVal);
+        EXPECT_EQ(++callCounter, pMockKernel->setArgSvmAllocCalls);
+        ++mockSvmManager->allocationsCounter;
+
         DebugManagerStateRestore stateRestorer;
         DebugManager.flags.EnableSharedSystemUsmSupport.set(1);
         mockSvmManager->freeSVMAlloc(nextPtrSvm);

Original file line number	Diff line number	Diff line change
`@@ -4900,6 +4900,11 @@ cl_int CL_API_CALL clSetKernelArgSVMPointer(cl_kernel kernel,`
`4900`	`4900`	`}`
`4901`	`4901`	`}`
`4902`	`4902`	`}`
	`4903`	`+ } else {`
	`4904`	`+ if (pMultiDeviceKernel->getKernelArguments()[argIndex].isSetToNullptr) {`
	`4905`	`+ TRACING_EXIT(clSetKernelArgSVMPointer, &retVal);`
	`4906`	`+ return CL_SUCCESS;`
	`4907`	`+ }`
`4903`	`4908`	`}`
`4904`	`4909`
`4905`	`4910`	`DBG_LOG_INPUTS("kernel", kernel, "argIndex", argIndex, "argValue", argValue);`