Merge pull request #805 from aarongreig/aaron/kernelSetArgIndirectionFix

Correct level of indirection used in KernelSetArgPointer calls.
oneapi-src · Jun 21, 2024 · 1e9b1b4 · 1e9b1b4
2 parents a011f09 + d8500a3
commit 1e9b1b4
Show file tree

Hide file tree

Showing 22 changed files with 44 additions and 47 deletions.
diff --git a/include/ur_api.h b/include/ur_api.h
@@ -5023,8 +5023,8 @@ urKernelSetArgPointer(
  ur_kernel_handle_t hKernel, ///< [in] handle of the kernel object
  uint32_t argIndex, ///< [in] argument index in range [0, num args - 1]
  const ur_kernel_arg_pointer_properties_t *pProperties, ///< [in][optional] pointer to USM pointer properties.
- const void *pArgValue ///< [in][optional] USM pointer to memory location holding the argument
- ///< value. If null then argument value is considered null.
+ const void *pArgValue ///< [in][optional] Pointer obtained by USM allocation or virtual memory
+ ///< mapping operation. If null then argument value is considered null.
 );
 
 ///////////////////////////////////////////////////////////////////////////////

diff --git a/scripts/core/kernel.yml b/scripts/core/kernel.yml
@@ -339,7 +339,7 @@ params:
  desc: "[in][optional] pointer to USM pointer properties."
  - type: "const void*"
  name: pArgValue
- desc: "[in][optional] USM pointer to memory location holding the argument value. If null then argument value is considered null."
+ desc: "[in][optional] Pointer obtained by USM allocation or virtual memory mapping operation. If null then argument value is considered null."
 returns:
  - $X_RESULT_ERROR_INVALID_KERNEL_ARGUMENT_INDEX
  - $X_RESULT_ERROR_INVALID_KERNEL_ARGUMENT_SIZE

diff --git a/source/adapters/cuda/kernel.cpp b/source/adapters/cuda/kernel.cpp
@@ -287,7 +287,8 @@ urKernelSetArgPointer(ur_kernel_handle_t hKernel, uint32_t argIndex,
  const ur_kernel_arg_pointer_properties_t *pProperties,
  const void *pArgValue) {
  std::ignore = pProperties;
- hKernel->setKernelArg(argIndex, sizeof(pArgValue), pArgValue);
+ // setKernelArg is expecting a pointer to our argument
+ hKernel->setKernelArg(argIndex, sizeof(pArgValue), &pArgValue);
  return UR_RESULT_SUCCESS;
 }
 

diff --git a/source/adapters/hip/kernel.cpp b/source/adapters/hip/kernel.cpp
@@ -275,7 +275,8 @@ urKernelGetSubGroupInfo(ur_kernel_handle_t hKernel, ur_device_handle_t hDevice,
 UR_APIEXPORT ur_result_t UR_APICALL urKernelSetArgPointer(
  ur_kernel_handle_t hKernel, uint32_t argIndex,
  const ur_kernel_arg_pointer_properties_t *, const void *pArgValue) {
- hKernel->setKernelArg(argIndex, sizeof(pArgValue), pArgValue);
+ // setKernelArg is expecting a pointer to our argument
+ hKernel->setKernelArg(argIndex, sizeof(pArgValue), &pArgValue);
  return UR_RESULT_SUCCESS;
 }
 

diff --git a/source/adapters/level_zero/kernel.cpp b/source/adapters/level_zero/kernel.cpp
@@ -1004,8 +1004,9 @@ UR_APIEXPORT ur_result_t UR_APICALL urKernelSetArgPointer(
 ) {
  std::ignore = Properties;
 
+ // KernelSetArgValue is expecting a pointer to the argument
  UR_CALL(urKernelSetArgValue(Kernel, ArgIndex, sizeof(const void *), nullptr,
- ArgValue));
+ &ArgValue));
  return UR_RESULT_SUCCESS;
 }
 

diff --git a/source/adapters/native_cpu/kernel.cpp b/source/adapters/native_cpu/kernel.cpp
@@ -213,9 +213,7 @@ urKernelSetArgPointer(ur_kernel_handle_t hKernel, uint32_t argIndex,
  UR_ASSERT(hKernel, UR_RESULT_ERROR_INVALID_NULL_HANDLE);
  UR_ASSERT(pArgValue, UR_RESULT_ERROR_INVALID_NULL_POINTER);
 
- auto ptrToPtr = reinterpret_cast<const intptr_t *>(pArgValue);
- auto derefPtr = reinterpret_cast<void *>(*ptrToPtr);
- hKernel->_args.push_back(derefPtr);
+ hKernel->_args.push_back(const_cast<void *>(pArgValue));
 
  return UR_RESULT_SUCCESS;
 }

diff --git a/source/adapters/null/ur_nullddi.cpp b/source/adapters/null/ur_nullddi.cpp
@@ -2446,8 +2446,8 @@ __urdlllocal ur_result_t UR_APICALL urKernelSetArgPointer(
  const ur_kernel_arg_pointer_properties_t
  *pProperties, ///< [in][optional] pointer to USM pointer properties.
  const void *
- pArgValue ///< [in][optional] USM pointer to memory location holding the argument
-  ///< value. If null then argument value is considered null.
+ pArgValue ///< [in][optional] Pointer obtained by USM allocation or virtual memory
+ ///< mapping operation. If null then argument value is considered null.
  ) try {
  ur_result_t result = UR_RESULT_SUCCESS;
 

diff --git a/source/adapters/opencl/kernel.cpp b/source/adapters/opencl/kernel.cpp
@@ -359,13 +359,9 @@ UR_APIEXPORT ur_result_t UR_APICALL urKernelSetArgPointer(
  cl_ext::SetKernelArgMemPointerName, &FuncPtr));
 
  if (FuncPtr) {
- /* OpenCL passes pointers by value not by reference. This means we need to
- * deref the arg to get the pointer value */
- auto PtrToPtr = reinterpret_cast<const intptr_t *>(pArgValue);
- auto DerefPtr = reinterpret_cast<void *>(*PtrToPtr);
  CL_RETURN_ON_FAILURE(FuncPtr(cl_adapter::cast<cl_kernel>(hKernel),
  cl_adapter::cast<cl_uint>(argIndex),
- DerefPtr));
+ pArgValue));
  }
 
  return UR_RESULT_SUCCESS;

diff --git a/source/loader/layers/sanitizer/asan_interceptor.cpp b/source/loader/layers/sanitizer/asan_interceptor.cpp
@@ -705,7 +705,7 @@ ur_result_t SanitizerInterceptor::prepareLaunch(
  char *ArgPointer = nullptr;
  UR_CALL(MemBuffer->getHandle(DeviceInfo->Handle, ArgPointer));
  ur_result_t URes = context.urDdiTable.Kernel.pfnSetArgPointer(
- Kernel, ArgIndex, nullptr, &ArgPointer);
+ Kernel, ArgIndex, nullptr, ArgPointer);
  if (URes != UR_RESULT_SUCCESS) {
  context.logger.error(
  "Failed to set buffer {} as the {} arg to kernel {}: {}",
@@ -722,7 +722,7 @@ ur_result_t SanitizerInterceptor::prepareLaunch(
  (void *)LaunchInfo.Data, LaunchInfo.Data->NumLocalArgs,
  (void *)LaunchInfo.Data->LocalArgs);
  ur_result_t URes = context.urDdiTable.Kernel.pfnSetArgPointer(
- Kernel, ArgNums - 1, nullptr, &LaunchInfo.Data);
+ Kernel, ArgNums - 1, nullptr, LaunchInfo.Data);
  if (URes != UR_RESULT_SUCCESS) {
  context.logger.error("Failed to set launch info: {}", URes);
  return URes;

diff --git a/source/loader/layers/tracing/ur_trcddi.cpp b/source/loader/layers/tracing/ur_trcddi.cpp
@@ -3167,8 +3167,8 @@ __urdlllocal ur_result_t UR_APICALL urKernelSetArgPointer(
  const ur_kernel_arg_pointer_properties_t
  *pProperties, ///< [in][optional] pointer to USM pointer properties.
  const void *
- pArgValue ///< [in][optional] USM pointer to memory location holding the argument
-  ///< value. If null then argument value is considered null.
+ pArgValue ///< [in][optional] Pointer obtained by USM allocation or virtual memory
+ ///< mapping operation. If null then argument value is considered null.
 ) {
  auto pfnSetArgPointer = context.urDdiTable.Kernel.pfnSetArgPointer;
 

diff --git a/source/loader/layers/validation/ur_valddi.cpp b/source/loader/layers/validation/ur_valddi.cpp
@@ -3589,8 +3589,8 @@ __urdlllocal ur_result_t UR_APICALL urKernelSetArgPointer(
  const ur_kernel_arg_pointer_properties_t
  *pProperties, ///< [in][optional] pointer to USM pointer properties.
  const void *
- pArgValue ///< [in][optional] USM pointer to memory location holding the argument
-  ///< value. If null then argument value is considered null.
+ pArgValue ///< [in][optional] Pointer obtained by USM allocation or virtual memory
+ ///< mapping operation. If null then argument value is considered null.
 ) {
  auto pfnSetArgPointer = context.urDdiTable.Kernel.pfnSetArgPointer;
 

diff --git a/source/loader/ur_ldrddi.cpp b/source/loader/ur_ldrddi.cpp
@@ -3230,8 +3230,8 @@ __urdlllocal ur_result_t UR_APICALL urKernelSetArgPointer(
  const ur_kernel_arg_pointer_properties_t
  *pProperties, ///< [in][optional] pointer to USM pointer properties.
  const void *
- pArgValue ///< [in][optional] USM pointer to memory location holding the argument
-  ///< value. If null then argument value is considered null.
+ pArgValue ///< [in][optional] Pointer obtained by USM allocation or virtual memory
+ ///< mapping operation. If null then argument value is considered null.
 ) {
  ur_result_t result = UR_RESULT_SUCCESS;
 

diff --git a/source/loader/ur_libapi.cpp b/source/loader/ur_libapi.cpp
@@ -3863,8 +3863,8 @@ ur_result_t UR_APICALL urKernelSetArgPointer(
  const ur_kernel_arg_pointer_properties_t
  *pProperties, ///< [in][optional] pointer to USM pointer properties.
  const void *
- pArgValue ///< [in][optional] USM pointer to memory location holding the argument
-  ///< value. If null then argument value is considered null.
+ pArgValue ///< [in][optional] Pointer obtained by USM allocation or virtual memory
+ ///< mapping operation. If null then argument value is considered null.
  ) try {
  auto pfnSetArgPointer = ur_lib::context->urDdiTable.Kernel.pfnSetArgPointer;
  if (nullptr == pfnSetArgPointer) {

diff --git a/source/ur_api.cpp b/source/ur_api.cpp
@@ -3284,8 +3284,8 @@ ur_result_t UR_APICALL urKernelSetArgPointer(
  const ur_kernel_arg_pointer_properties_t
  *pProperties, ///< [in][optional] pointer to USM pointer properties.
  const void *
- pArgValue ///< [in][optional] USM pointer to memory location holding the argument
-  ///< value. If null then argument value is considered null.
+ pArgValue ///< [in][optional] Pointer obtained by USM allocation or virtual memory
+ ///< mapping operation. If null then argument value is considered null.
 ) {
  ur_result_t result = UR_RESULT_SUCCESS;
  return result;

diff --git a/test/conformance/enqueue/urEnqueueKernelLaunch.cpp b/test/conformance/enqueue/urEnqueueKernelLaunch.cpp
@@ -364,7 +364,7 @@ TEST_P(urEnqueueKernelLaunchWithVirtualMemory, Success) {
  size_t global_size = alloc_size / sizeof(uint32_t);
  uint32_t fill_val = 42;
 
- ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, &virtual_ptr));
+ ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, virtual_ptr));
  ASSERT_SUCCESS(
  urKernelSetArgValue(kernel, 1, sizeof(fill_val), nullptr, &fill_val));
 
@@ -516,7 +516,7 @@ TEST_P(urEnqueueKernelLaunchUSMLinkedList, Success) {
  }
 
  // Run kernel which will iterate the list and modify the values
- ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, &list_head));
+ ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, list_head));
  ASSERT_SUCCESS(urEnqueueKernelLaunch(queue, kernel, 1, &global_offset,
  &global_size, nullptr, 0, nullptr,
  nullptr));

diff --git a/test/conformance/exp_command_buffer/invalid_update.cpp b/test/conformance/exp_command_buffer/invalid_update.cpp
@@ -28,7 +28,7 @@ struct InvalidUpdateTest
  std::memset(shared_ptr, 0, allocation_size);
 
  // Index 0 is output
- ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, &shared_ptr));
+ ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, shared_ptr));
  // Index 1 is input scalar
  ASSERT_SUCCESS(
  urKernelSetArgValue(kernel, 1, sizeof(val), nullptr, &val));

diff --git a/test/conformance/exp_command_buffer/ndrange_update.cpp b/test/conformance/exp_command_buffer/ndrange_update.cpp
@@ -28,7 +28,7 @@ struct NDRangeUpdateTest
  ASSERT_NE(shared_ptr, nullptr);
  std::memset(shared_ptr, 0, allocation_size);
 
- ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, &shared_ptr));
+ ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, shared_ptr));
 
  // Add a 3 dimension kernel command to command-buffer and close
  // command-buffer

diff --git a/test/conformance/exp_command_buffer/usm_fill_kernel_update.cpp b/test/conformance/exp_command_buffer/usm_fill_kernel_update.cpp
@@ -29,7 +29,7 @@ struct USMFillCommandTest
  std::memset(shared_ptr, 0, allocation_size);
 
  // Index 0 is output
- ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, &shared_ptr));
+ ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, shared_ptr));
  // Index 1 is input scalar
  ASSERT_SUCCESS(
  urKernelSetArgValue(kernel, 1, sizeof(val), nullptr, &val));
@@ -223,7 +223,7 @@ struct USMMultipleFillCommandTest
  // kernel output.
  void *offset_ptr = (uint32_t *)shared_ptr + (k * elements);
  ASSERT_SUCCESS(
- urKernelSetArgPointer(kernel, 0, nullptr, &offset_ptr));
+ urKernelSetArgPointer(kernel, 0, nullptr, offset_ptr));
 
  // Each kernel has a unique fill value
  uint32_t fill_val = val + k;

diff --git a/test/conformance/exp_command_buffer/usm_saxpy_kernel_update.cpp b/test/conformance/exp_command_buffer/usm_saxpy_kernel_update.cpp
@@ -36,15 +36,15 @@ struct USMSaxpyKernelTestBase
 
  // Index 0 is output
  ASSERT_SUCCESS(
- urKernelSetArgPointer(kernel, 0, nullptr, &shared_ptrs[0]));
+ urKernelSetArgPointer(kernel, 0, nullptr, shared_ptrs[0]));
  // Index 1 is A
  ASSERT_SUCCESS(urKernelSetArgValue(kernel, 1, sizeof(A), nullptr, &A));
  // Index 2 is X
  ASSERT_SUCCESS(
- urKernelSetArgPointer(kernel, 2, nullptr, &shared_ptrs[1]));
+ urKernelSetArgPointer(kernel, 2, nullptr, shared_ptrs[1]));
  // Index 3 is Y
  ASSERT_SUCCESS(
- urKernelSetArgPointer(kernel, 3, nullptr, &shared_ptrs[2]));
+ urKernelSetArgPointer(kernel, 3, nullptr, shared_ptrs[2]));
  }
 
  void Validate(uint32_t *output, uint32_t *X, uint32_t *Y, uint32_t A,

diff --git a/test/conformance/integration/QueueEmptyStatus.cpp b/test/conformance/integration/QueueEmptyStatus.cpp
@@ -39,7 +39,7 @@ struct QueueEmptyStatusTestWithParam : uur::IntegrationQueueTestWithParam {
  ArraySize * sizeof(uint32_t), 0, nullptr, &Event));
  ASSERT_NO_FATAL_FAILURE(submitBarrierIfNeeded(Event));
 
- ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, &SharedMem));
+ ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, SharedMem));
 
  constexpr size_t global_offset = 0;
  constexpr size_t n_dimensions = 1;

diff --git a/test/conformance/integration/QueueUSM.cpp b/test/conformance/integration/QueueUSM.cpp
@@ -88,8 +88,8 @@ TEST_P(QueueUSMTestWithParam, QueueUSMTest) {
 
  for (uint32_t i = 0; i < NumIterations; ++i) {
  /* Copy from DeviceMem2 to DeviceMem1 and multiply by 2 */
- ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, &DeviceMem1));
- ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 1, nullptr, &DeviceMem2));
+ ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, DeviceMem1));
+ ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 1, nullptr, DeviceMem2));
 
  ASSERT_SUCCESS(urEnqueueKernelLaunch(Queue, kernel, NDimensions,
  &GlobalOffset, &ArraySize, nullptr,
@@ -99,8 +99,8 @@ TEST_P(QueueUSMTestWithParam, QueueUSMTest) {
  CurValueMem2 = CurValueMem1 * 2;
 
  /* Copy from DeviceMem1 to DeviceMem2 and multiply by 2 */
- ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, &DeviceMem2));
- ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 1, nullptr, &DeviceMem1));
+ ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, DeviceMem2));
+ ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 1, nullptr, DeviceMem1));
 
  ASSERT_SUCCESS(urEnqueueKernelLaunch(Queue, kernel, NDimensions,
  &GlobalOffset, &ArraySize, nullptr,

diff --git a/test/conformance/kernel/urKernelSetArgPointer.cpp b/test/conformance/kernel/urKernelSetArgPointer.cpp
@@ -42,7 +42,7 @@ TEST_P(urKernelSetArgPointerTest, SuccessHost) {
  &allocation));
  ASSERT_NE(allocation, nullptr);
 
- ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, &allocation));
+ ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, allocation));
  ASSERT_SUCCESS(
  urKernelSetArgValue(kernel, 1, sizeof(data), nullptr, &data));
  Launch1DRange(array_size);
@@ -60,7 +60,7 @@ TEST_P(urKernelSetArgPointerTest, SuccessDevice) {
  allocation_size, &allocation));
  ASSERT_NE(allocation, nullptr);
 
- ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, &allocation));
+ ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, allocation));
  ASSERT_SUCCESS(
  urKernelSetArgValue(kernel, 1, sizeof(data), nullptr, &data));
  Launch1DRange(array_size);
@@ -87,7 +87,7 @@ TEST_P(urKernelSetArgPointerTest, SuccessShared) {
  allocation_size, &allocation));
  ASSERT_NE(allocation, nullptr);
 
- ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, &allocation));
+ ASSERT_SUCCESS(urKernelSetArgPointer(kernel, 0, nullptr, allocation));
  ASSERT_SUCCESS(
  urKernelSetArgValue(kernel, 1, sizeof(data), nullptr, &data));
  Launch1DRange(array_size);
@@ -138,7 +138,7 @@ UUR_INSTANTIATE_KERNEL_TEST_SUITE_P(urKernelSetArgPointerNegativeTest);
 
 TEST_P(urKernelSetArgPointerNegativeTest, InvalidNullHandleKernel) {
  ASSERT_EQ_RESULT(UR_RESULT_ERROR_INVALID_NULL_HANDLE,
- urKernelSetArgPointer(nullptr, 0, nullptr, &allocation));
+ urKernelSetArgPointer(nullptr, 0, nullptr, allocation));
 }
 
 TEST_P(urKernelSetArgPointerNegativeTest, InvalidKernelArgumentIndex) {
@@ -149,5 +149,5 @@ TEST_P(urKernelSetArgPointerNegativeTest, InvalidKernelArgumentIndex) {
 
  ASSERT_EQ_RESULT(UR_RESULT_ERROR_INVALID_KERNEL_ARGUMENT_INDEX,
  urKernelSetArgPointer(kernel, num_kernel_args + 1, nullptr,
- &allocation));
+ allocation));
 }