From 41f3626f8b300cef24c06d9e8b7cf53029a4330a Mon Sep 17 00:00:00 2001 From: Michael Halkenhaeuser Date: Wed, 16 Aug 2023 06:38:39 -0400 Subject: [PATCH] [OpenMP][OMPT] Fix reported target pointer for data alloc callback This patch fixes: https://github.com/llvm/llvm-project/issues/64671 DataOp EMI callbacks would not report the correct target pointer. This is now alleviated by passing a `void**` into the function which emits the actual callback, then evaluating that pointer. Note: Since this is only done after the pointer has been properly updated, only `endpoint=2` callbacks will show a non-null value. Reviewed By: dhruvachak, jdoerfert Differential Revision: https://reviews.llvm.org/D157996 --- openmp/libomptarget/src/OmptCallback.cpp | 12 +++++++----- openmp/libomptarget/src/OmptInterface.h | 8 ++++---- openmp/libomptarget/src/device.cpp | 6 ++++-- .../libomptarget/test/ompt/veccopy_disallow_both.c | 4 ++++ openmp/libomptarget/test/ompt/veccopy_emi.c | 4 ++++ openmp/libomptarget/test/ompt/veccopy_emi_map.c | 4 ++++ 6 files changed, 27 insertions(+), 11 deletions(-) diff --git a/openmp/libomptarget/src/OmptCallback.cpp b/openmp/libomptarget/src/OmptCallback.cpp index cd44d0903be9..4882a762adbf 100644 --- a/openmp/libomptarget/src/OmptCallback.cpp +++ b/openmp/libomptarget/src/OmptCallback.cpp @@ -71,7 +71,8 @@ static uint64_t createRegionId() { } void Interface::beginTargetDataAlloc(int64_t DeviceId, void *HstPtrBegin, - size_t Size, void *Code) { + void **TgtPtrBegin, size_t Size, + void *Code) { beginTargetDataOperation(); if (ompt_callback_target_data_op_emi_fn) { // HostOpId will be set by the tool. Invoke the tool supplied data op EMI @@ -79,7 +80,7 @@ void Interface::beginTargetDataAlloc(int64_t DeviceId, void *HstPtrBegin, ompt_callback_target_data_op_emi_fn( ompt_scope_begin, TargetTaskData, &TargetData, &TargetRegionOpId, ompt_target_data_alloc, HstPtrBegin, - /* SrcDeviceNum */ omp_get_initial_device(), /* TgtPtrBegin */ nullptr, + /* SrcDeviceNum */ omp_get_initial_device(), *TgtPtrBegin, /* TgtDeviceNum */ DeviceId, Size, Code); } else if (ompt_callback_target_data_op_fn) { // HostOpId is set by the runtime @@ -87,13 +88,14 @@ void Interface::beginTargetDataAlloc(int64_t DeviceId, void *HstPtrBegin, // Invoke the tool supplied data op callback ompt_callback_target_data_op_fn( TargetData.value, HostOpId, ompt_target_data_alloc, HstPtrBegin, - /* SrcDeviceNum */ omp_get_initial_device(), /* TgtPtrBegin */ nullptr, + /* SrcDeviceNum */ omp_get_initial_device(), *TgtPtrBegin, /* TgtDeviceNum */ DeviceId, Size, Code); } } void Interface::endTargetDataAlloc(int64_t DeviceId, void *HstPtrBegin, - size_t Size, void *Code) { + void **TgtPtrBegin, size_t Size, + void *Code) { // Only EMI callback handles end scope if (ompt_callback_target_data_op_emi_fn) { // HostOpId will be set by the tool. Invoke the tool supplied data op EMI @@ -101,7 +103,7 @@ void Interface::endTargetDataAlloc(int64_t DeviceId, void *HstPtrBegin, ompt_callback_target_data_op_emi_fn( ompt_scope_end, TargetTaskData, &TargetData, &TargetRegionOpId, ompt_target_data_alloc, HstPtrBegin, - /* SrcDeviceNum */ omp_get_initial_device(), /* TgtPtrBegin */ nullptr, + /* SrcDeviceNum */ omp_get_initial_device(), *TgtPtrBegin, /* TgtDeviceNum */ DeviceId, Size, Code); } endTargetDataOperation(); diff --git a/openmp/libomptarget/src/OmptInterface.h b/openmp/libomptarget/src/OmptInterface.h index c3a52969bf80..178cedacf4a5 100644 --- a/openmp/libomptarget/src/OmptInterface.h +++ b/openmp/libomptarget/src/OmptInterface.h @@ -47,12 +47,12 @@ static ompt_get_target_task_data_t ompt_get_target_task_data_fn; class Interface { public: /// Top-level function for invoking callback before device data allocation - void beginTargetDataAlloc(int64_t DeviceId, void *TgtPtrBegin, size_t Size, - void *Code); + void beginTargetDataAlloc(int64_t DeviceId, void *HstPtrBegin, + void **TgtPtrBegin, size_t Size, void *Code); /// Top-level function for invoking callback after device data allocation - void endTargetDataAlloc(int64_t DeviceId, void *TgtPtrBegin, size_t Size, - void *Code); + void endTargetDataAlloc(int64_t DeviceId, void *HstPtrBegin, + void **TgtPtrBegin, size_t Size, void *Code); /// Top-level function for invoking callback before data submit void beginTargetDataSubmit(int64_t DeviceId, void *HstPtrBegin, diff --git a/openmp/libomptarget/src/device.cpp b/openmp/libomptarget/src/device.cpp index f4d2038c5cd3..ca5f48a4e49d 100644 --- a/openmp/libomptarget/src/device.cpp +++ b/openmp/libomptarget/src/device.cpp @@ -580,12 +580,14 @@ __tgt_target_table *DeviceTy::loadBinary(void *Img) { void *DeviceTy::allocData(int64_t Size, void *HstPtr, int32_t Kind) { /// RAII to establish tool anchors before and after data allocation + void *TargetPtr = nullptr; OMPT_IF_BUILT(InterfaceRAII TargetDataAllocRAII( RegionInterface.getCallbacks(), - RTLDeviceID, HstPtr, Size, + RTLDeviceID, HstPtr, &TargetPtr, Size, /* CodePtr */ OMPT_GET_RETURN_ADDRESS(0));) - return RTL->data_alloc(RTLDeviceID, Size, HstPtr, Kind); + TargetPtr = RTL->data_alloc(RTLDeviceID, Size, HstPtr, Kind); + return TargetPtr; } int32_t DeviceTy::deleteData(void *TgtAllocBegin, int32_t Kind) { diff --git a/openmp/libomptarget/test/ompt/veccopy_disallow_both.c b/openmp/libomptarget/test/ompt/veccopy_disallow_both.c index 6fdcfdb03537..9d3498dc72d2 100644 --- a/openmp/libomptarget/test/ompt/veccopy_disallow_both.c +++ b/openmp/libomptarget/test/ompt/veccopy_disallow_both.c @@ -63,10 +63,12 @@ int main() { /// CHECK: Callback Target EMI: kind=1 endpoint=1 /// CHECK: Callback DataOp EMI: endpoint=1 optype=1 /// CHECK: Callback DataOp EMI: endpoint=2 optype=1 +/// CHECK-NOT: dest=(nil) /// CHECK: Callback DataOp EMI: endpoint=1 optype=2 /// CHECK: Callback DataOp EMI: endpoint=2 optype=2 /// CHECK: Callback DataOp EMI: endpoint=1 optype=1 /// CHECK: Callback DataOp EMI: endpoint=2 optype=1 +/// CHECK-NOT: dest=(nil) /// CHECK: Callback DataOp EMI: endpoint=1 optype=2 /// CHECK: Callback DataOp EMI: endpoint=2 optype=2 /// CHECK: Callback Submit: target_id=[[TARGET_ID:[0-9]+]] host_op_id=[[HOST_OP_ID:[0-9]+]] req_num_teams=1 @@ -82,10 +84,12 @@ int main() { /// CHECK: Callback Target EMI: kind=1 endpoint=1 /// CHECK: Callback DataOp EMI: endpoint=1 optype=1 /// CHECK: Callback DataOp EMI: endpoint=2 optype=1 +/// CHECK-NOT: dest=(nil) /// CHECK: Callback DataOp EMI: endpoint=1 optype=2 /// CHECK: Callback DataOp EMI: endpoint=2 optype=2 /// CHECK: Callback DataOp EMI: endpoint=1 optype=1 /// CHECK: Callback DataOp EMI: endpoint=2 optype=1 +/// CHECK-NOT: dest=(nil) /// CHECK: Callback DataOp EMI: endpoint=1 optype=2 /// CHECK: Callback DataOp EMI: endpoint=2 optype=2 /// CHECK: Callback Submit: target_id=[[TARGET_ID:[0-9]+]] host_op_id=[[HOST_OP_ID:[0-9]+]] req_num_teams=0 diff --git a/openmp/libomptarget/test/ompt/veccopy_emi.c b/openmp/libomptarget/test/ompt/veccopy_emi.c index f15dfb18da46..5adf302bd1ff 100644 --- a/openmp/libomptarget/test/ompt/veccopy_emi.c +++ b/openmp/libomptarget/test/ompt/veccopy_emi.c @@ -61,10 +61,12 @@ int main() { /// CHECK: Callback Target EMI: kind=1 endpoint=1 /// CHECK: Callback DataOp EMI: endpoint=1 optype=1 /// CHECK: Callback DataOp EMI: endpoint=2 optype=1 +/// CHECK-NOT: dest=(nil) /// CHECK: Callback DataOp EMI: endpoint=1 optype=2 /// CHECK: Callback DataOp EMI: endpoint=2 optype=2 /// CHECK: Callback DataOp EMI: endpoint=1 optype=1 /// CHECK: Callback DataOp EMI: endpoint=2 optype=1 +/// CHECK-NOT: dest=(nil) /// CHECK: Callback DataOp EMI: endpoint=1 optype=2 /// CHECK: Callback DataOp EMI: endpoint=2 optype=2 /// CHECK: Callback Submit EMI: endpoint=1 req_num_teams=1 @@ -81,10 +83,12 @@ int main() { /// CHECK: Callback Target EMI: kind=1 endpoint=1 /// CHECK: Callback DataOp EMI: endpoint=1 optype=1 /// CHECK: Callback DataOp EMI: endpoint=2 optype=1 +/// CHECK-NOT: dest=(nil) /// CHECK: Callback DataOp EMI: endpoint=1 optype=2 /// CHECK: Callback DataOp EMI: endpoint=2 optype=2 /// CHECK: Callback DataOp EMI: endpoint=1 optype=1 /// CHECK: Callback DataOp EMI: endpoint=2 optype=1 +/// CHECK-NOT: dest=(nil) /// CHECK: Callback DataOp EMI: endpoint=1 optype=2 /// CHECK: Callback DataOp EMI: endpoint=2 optype=2 /// CHECK: Callback Submit EMI: endpoint=1 req_num_teams=0 diff --git a/openmp/libomptarget/test/ompt/veccopy_emi_map.c b/openmp/libomptarget/test/ompt/veccopy_emi_map.c index af0743f0369c..edf08325c41b 100644 --- a/openmp/libomptarget/test/ompt/veccopy_emi_map.c +++ b/openmp/libomptarget/test/ompt/veccopy_emi_map.c @@ -62,10 +62,12 @@ int main() { /// CHECK: Callback Target EMI: kind=1 endpoint=1 /// CHECK: Callback DataOp EMI: endpoint=1 optype=1 /// CHECK: Callback DataOp EMI: endpoint=2 optype=1 +/// CHECK-NOT: dest=(nil) /// CHECK: Callback DataOp EMI: endpoint=1 optype=2 /// CHECK: Callback DataOp EMI: endpoint=2 optype=2 /// CHECK: Callback DataOp EMI: endpoint=1 optype=1 /// CHECK: Callback DataOp EMI: endpoint=2 optype=1 +/// CHECK-NOT: dest=(nil) /// CHECK: Callback DataOp EMI: endpoint=1 optype=2 /// CHECK: Callback DataOp EMI: endpoint=2 optype=2 /// CHECK: Callback Submit EMI: endpoint=1 req_num_teams=1 @@ -82,10 +84,12 @@ int main() { /// CHECK: Callback Target EMI: kind=1 endpoint=1 /// CHECK: Callback DataOp EMI: endpoint=1 optype=1 /// CHECK: Callback DataOp EMI: endpoint=2 optype=1 +/// CHECK-NOT: dest=(nil) /// CHECK: Callback DataOp EMI: endpoint=1 optype=2 /// CHECK: Callback DataOp EMI: endpoint=2 optype=2 /// CHECK: Callback DataOp EMI: endpoint=1 optype=1 /// CHECK: Callback DataOp EMI: endpoint=2 optype=1 +/// CHECK-NOT: dest=(nil) /// CHECK: Callback DataOp EMI: endpoint=1 optype=2 /// CHECK: Callback DataOp EMI: endpoint=2 optype=2 /// CHECK: Callback Submit EMI: endpoint=1 req_num_teams=0