-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[MLIR][OpenMP] Add OpenMPToLLVMIRTranslation support for is_device_ptr #169367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: users/Akash/is_device_ptr_lower
Are you sure you want to change the base?
[MLIR][OpenMP] Add OpenMPToLLVMIRTranslation support for is_device_ptr #169367
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for the OpenMP is_device_ptr clause in the MLIR to LLVM IR translation for target regions. The is_device_ptr clause allows device pointers (allocated via OpenMP runtime APIs) to be used directly in target regions without implicit mapping.
Key Changes:
- Implemented
is_device_ptrhandling incollectMapDataFromMapOperandsby detecting thestorageflag and setting appropriate LLVM map flags - Removed the unimplemented clause check for
is_device_ptrinomp.targetoperations - Added comprehensive Fortran tests validating the end-to-end functionality
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp |
Implemented is_device_ptr translation by detecting variables with the storage flag and setting OMP_MAP_TARGET_PARAM and OMP_MAP_LITERAL flags, along with the Pointer device info type |
mlir/test/Target/LLVMIR/openmp-todo.mlir |
Removed TODO test for is_device_ptr since it's now implemented |
offload/test/offloading/fortran/target-is-device-ptr.f90 |
Added comprehensive Fortran test validating device pointer allocation and usage with is_device_ptr clause |
flang/test/Integration/OpenMP/map-types-and-sizes.f90 |
Added test case verifying correct map type value (288) for is_device_ptr variables |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-offload Author: Akash Banerjee (TIFitis) ChangesThis PR adds support for the OpenMP Full diff: https://github.com/llvm/llvm-project/pull/169367.diff 4 Files Affected:
diff --git a/flang/test/Integration/OpenMP/map-types-and-sizes.f90 b/flang/test/Integration/OpenMP/map-types-and-sizes.f90
index 44a049f5ac510..85434460bbea6 100644
--- a/flang/test/Integration/OpenMP/map-types-and-sizes.f90
+++ b/flang/test/Integration/OpenMP/map-types-and-sizes.f90
@@ -33,6 +33,15 @@ subroutine mapType_array
!$omp end target
end subroutine mapType_array
+!CHECK: @.offload_sizes{{.*}} = private unnamed_addr constant [1 x i64] [i64 8]
+!CHECK: @.offload_maptypes{{.*}} = private unnamed_addr constant [1 x i64] [i64 288]
+subroutine mapType_is_device_ptr
+ use iso_c_binding, only : c_ptr
+ type(c_ptr) :: p
+ !$omp target is_device_ptr(p)
+ !$omp end target
+end subroutine mapType_is_device_ptr
+
!CHECK: @.offload_sizes{{.*}} = private unnamed_addr constant [4 x i64] [i64 0, i64 24, i64 8, i64 0]
!CHECK: @.offload_maptypes{{.*}} = private unnamed_addr constant [4 x i64] [i64 32, i64 281474976711169, i64 281474976711171, i64 281474976711187]
subroutine mapType_ptr
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index f28454075f1d3..00c4c351caa4e 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -332,10 +332,7 @@ static LogicalResult checkImplementationStatus(Operation &op) {
op.getInReductionSyms())
result = todo("in_reduction");
};
- auto checkIsDevicePtr = [&todo](auto op, LogicalResult &result) {
- if (!op.getIsDevicePtrVars().empty())
- result = todo("is_device_ptr");
- };
+ auto checkIsDevicePtr = [](auto, LogicalResult &) {};
auto checkLinear = [&todo](auto op, LogicalResult &result) {
if (!op.getLinearVars().empty() || !op.getLinearStepVars().empty())
result = todo("linear");
@@ -3996,6 +3993,8 @@ static void collectMapDataFromMapOperands(
llvm::Value *origValue = moduleTranslation.lookupValue(offloadPtr);
auto mapType = convertClauseMapFlags(mapOp.getMapType());
auto mapTypeAlways = llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ALWAYS;
+ bool isDevicePtr = (mapOp.getMapType() & omp::ClauseMapFlags::storage) ==
+ omp::ClauseMapFlags::storage;
mapData.OriginalValue.push_back(origValue);
mapData.BasePointers.push_back(origValue);
@@ -4006,7 +4005,12 @@ static void collectMapDataFromMapOperands(
mapData.Sizes.push_back(
builder.getInt64(dl.getTypeSize(mapOp.getVarType())));
mapData.MapClause.push_back(mapOp.getOperation());
- if (llvm::to_underlying(mapType & mapTypeAlways)) {
+ if (isDevicePtr) {
+ mapType |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TARGET_PARAM;
+ mapType |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_LITERAL;
+ mapData.Types.push_back(mapType);
+ mapData.Mappers.push_back(nullptr);
+ } else if (llvm::to_underlying(mapType & mapTypeAlways)) {
// Descriptors are mapped with the ALWAYS flag, since they can get
// rematerialized, so the address of the decriptor for a given object
// may change from one place to another.
@@ -4029,7 +4033,8 @@ static void collectMapDataFromMapOperands(
mapData.Names.push_back(LLVM::createMappingInformation(
mapOp.getLoc(), *moduleTranslation.getOpenMPBuilder()));
mapData.DevicePointers.push_back(
- llvm::OpenMPIRBuilder::DeviceInfoTy::Address);
+ isDevicePtr ? llvm::OpenMPIRBuilder::DeviceInfoTy::Pointer
+ : llvm::OpenMPIRBuilder::DeviceInfoTy::Address);
mapData.IsAMapping.push_back(false);
mapData.IsAMember.push_back(checkIsAMember(hasDevAddrOperands, mapOp));
}
diff --git a/mlir/test/Target/LLVMIR/openmp-todo.mlir b/mlir/test/Target/LLVMIR/openmp-todo.mlir
index af6d254cfd3c3..0704008aa7135 100644
--- a/mlir/test/Target/LLVMIR/openmp-todo.mlir
+++ b/mlir/test/Target/LLVMIR/openmp-todo.mlir
@@ -238,17 +238,6 @@ llvm.func @target_in_reduction(%x : !llvm.ptr) {
// -----
-llvm.func @target_is_device_ptr(%x : !llvm.ptr) {
- // expected-error@below {{not yet implemented: Unhandled clause is_device_ptr in omp.target operation}}
- // expected-error@below {{LLVM Translation failed for operation: omp.target}}
- omp.target is_device_ptr(%x : !llvm.ptr) {
- omp.terminator
- }
- llvm.return
-}
-
-// -----
-
llvm.func @target_enter_data_depend(%x: !llvm.ptr) {
// expected-error@below {{not yet implemented: Unhandled clause depend in omp.target_enter_data operation}}
// expected-error@below {{LLVM Translation failed for operation: omp.target_enter_data}}
diff --git a/offload/test/offloading/fortran/target-is-device-ptr.f90 b/offload/test/offloading/fortran/target-is-device-ptr.f90
new file mode 100644
index 0000000000000..6010d59113498
--- /dev/null
+++ b/offload/test/offloading/fortran/target-is-device-ptr.f90
@@ -0,0 +1,79 @@
+! Validate that a device pointer allocated via OpenMP runtime APIs can be
+! consumed by a TARGET region using the is_device_ptr clause.
+! REQUIRES: flang, amdgcn-amd-amdhsa
+! UNSUPPORTED: nvptx64-nvidia-cuda
+! UNSUPPORTED: nvptx64-nvidia-cuda-LTO
+! UNSUPPORTED: aarch64-unknown-linux-gnu
+! UNSUPPORTED: aarch64-unknown-linux-gnu-LTO
+! UNSUPPORTED: x86_64-unknown-linux-gnu
+! UNSUPPORTED: x86_64-unknown-linux-gnu-LTO
+
+! RUN: %libomptarget-compile-fortran-run-and-check-generic
+
+program is_device_ptr_target
+ use omp_lib
+ use iso_c_binding
+ implicit none
+
+ integer, parameter :: n = 4
+ integer, target :: host(n)
+ type(c_ptr) :: device_ptr
+ integer(c_int) :: rc
+ integer :: i
+
+ do i = 1, n
+ host(i) = i
+ end do
+
+ device_ptr = omp_target_alloc(int(n, c_size_t) * int(c_sizeof(host(1)), c_size_t), &
+ omp_get_default_device())
+ if (.not. c_associated(device_ptr)) then
+ print *, "device alloc failed"
+ stop 1
+ end if
+
+ rc = omp_target_memcpy(device_ptr, c_loc(host), &
+ int(n, c_size_t) * int(c_sizeof(host(1)), c_size_t), &
+ 0_c_size_t, 0_c_size_t, &
+ omp_get_default_device(), omp_get_initial_device())
+ if (rc .ne. 0) then
+ print *, "host->device memcpy failed"
+ call omp_target_free(device_ptr, omp_get_default_device())
+ stop 1
+ end if
+
+ call fill_on_device(device_ptr)
+
+ rc = omp_target_memcpy(c_loc(host), device_ptr, &
+ int(n, c_size_t) * int(c_sizeof(host(1)), c_size_t), &
+ 0_c_size_t, 0_c_size_t, &
+ omp_get_initial_device(), omp_get_default_device())
+ call omp_target_free(device_ptr, omp_get_default_device())
+
+ if (rc .ne. 0) then
+ print *, "device->host memcpy failed"
+ stop 1
+ end if
+
+ if (all(host == [2, 4, 6, 8])) then
+ print *, "PASS"
+ else
+ print *, "FAIL", host
+ end if
+
+contains
+ subroutine fill_on_device(ptr)
+ type(c_ptr) :: ptr
+ integer, pointer :: p(:)
+ call c_f_pointer(ptr, p, [n])
+
+ !$omp target is_device_ptr(ptr)
+ p(1) = 2
+ p(2) = 4
+ p(3) = 6
+ p(4) = 8
+ !$omp end target
+ end subroutine fill_on_device
+end program is_device_ptr_target
+
+!CHECK: PASS
|
|
@llvm/pr-subscribers-mlir-openmp Author: Akash Banerjee (TIFitis) ChangesThis PR adds support for the OpenMP Full diff: https://github.com/llvm/llvm-project/pull/169367.diff 4 Files Affected:
diff --git a/flang/test/Integration/OpenMP/map-types-and-sizes.f90 b/flang/test/Integration/OpenMP/map-types-and-sizes.f90
index 44a049f5ac510..85434460bbea6 100644
--- a/flang/test/Integration/OpenMP/map-types-and-sizes.f90
+++ b/flang/test/Integration/OpenMP/map-types-and-sizes.f90
@@ -33,6 +33,15 @@ subroutine mapType_array
!$omp end target
end subroutine mapType_array
+!CHECK: @.offload_sizes{{.*}} = private unnamed_addr constant [1 x i64] [i64 8]
+!CHECK: @.offload_maptypes{{.*}} = private unnamed_addr constant [1 x i64] [i64 288]
+subroutine mapType_is_device_ptr
+ use iso_c_binding, only : c_ptr
+ type(c_ptr) :: p
+ !$omp target is_device_ptr(p)
+ !$omp end target
+end subroutine mapType_is_device_ptr
+
!CHECK: @.offload_sizes{{.*}} = private unnamed_addr constant [4 x i64] [i64 0, i64 24, i64 8, i64 0]
!CHECK: @.offload_maptypes{{.*}} = private unnamed_addr constant [4 x i64] [i64 32, i64 281474976711169, i64 281474976711171, i64 281474976711187]
subroutine mapType_ptr
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index f28454075f1d3..00c4c351caa4e 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -332,10 +332,7 @@ static LogicalResult checkImplementationStatus(Operation &op) {
op.getInReductionSyms())
result = todo("in_reduction");
};
- auto checkIsDevicePtr = [&todo](auto op, LogicalResult &result) {
- if (!op.getIsDevicePtrVars().empty())
- result = todo("is_device_ptr");
- };
+ auto checkIsDevicePtr = [](auto, LogicalResult &) {};
auto checkLinear = [&todo](auto op, LogicalResult &result) {
if (!op.getLinearVars().empty() || !op.getLinearStepVars().empty())
result = todo("linear");
@@ -3996,6 +3993,8 @@ static void collectMapDataFromMapOperands(
llvm::Value *origValue = moduleTranslation.lookupValue(offloadPtr);
auto mapType = convertClauseMapFlags(mapOp.getMapType());
auto mapTypeAlways = llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ALWAYS;
+ bool isDevicePtr = (mapOp.getMapType() & omp::ClauseMapFlags::storage) ==
+ omp::ClauseMapFlags::storage;
mapData.OriginalValue.push_back(origValue);
mapData.BasePointers.push_back(origValue);
@@ -4006,7 +4005,12 @@ static void collectMapDataFromMapOperands(
mapData.Sizes.push_back(
builder.getInt64(dl.getTypeSize(mapOp.getVarType())));
mapData.MapClause.push_back(mapOp.getOperation());
- if (llvm::to_underlying(mapType & mapTypeAlways)) {
+ if (isDevicePtr) {
+ mapType |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TARGET_PARAM;
+ mapType |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_LITERAL;
+ mapData.Types.push_back(mapType);
+ mapData.Mappers.push_back(nullptr);
+ } else if (llvm::to_underlying(mapType & mapTypeAlways)) {
// Descriptors are mapped with the ALWAYS flag, since they can get
// rematerialized, so the address of the decriptor for a given object
// may change from one place to another.
@@ -4029,7 +4033,8 @@ static void collectMapDataFromMapOperands(
mapData.Names.push_back(LLVM::createMappingInformation(
mapOp.getLoc(), *moduleTranslation.getOpenMPBuilder()));
mapData.DevicePointers.push_back(
- llvm::OpenMPIRBuilder::DeviceInfoTy::Address);
+ isDevicePtr ? llvm::OpenMPIRBuilder::DeviceInfoTy::Pointer
+ : llvm::OpenMPIRBuilder::DeviceInfoTy::Address);
mapData.IsAMapping.push_back(false);
mapData.IsAMember.push_back(checkIsAMember(hasDevAddrOperands, mapOp));
}
diff --git a/mlir/test/Target/LLVMIR/openmp-todo.mlir b/mlir/test/Target/LLVMIR/openmp-todo.mlir
index af6d254cfd3c3..0704008aa7135 100644
--- a/mlir/test/Target/LLVMIR/openmp-todo.mlir
+++ b/mlir/test/Target/LLVMIR/openmp-todo.mlir
@@ -238,17 +238,6 @@ llvm.func @target_in_reduction(%x : !llvm.ptr) {
// -----
-llvm.func @target_is_device_ptr(%x : !llvm.ptr) {
- // expected-error@below {{not yet implemented: Unhandled clause is_device_ptr in omp.target operation}}
- // expected-error@below {{LLVM Translation failed for operation: omp.target}}
- omp.target is_device_ptr(%x : !llvm.ptr) {
- omp.terminator
- }
- llvm.return
-}
-
-// -----
-
llvm.func @target_enter_data_depend(%x: !llvm.ptr) {
// expected-error@below {{not yet implemented: Unhandled clause depend in omp.target_enter_data operation}}
// expected-error@below {{LLVM Translation failed for operation: omp.target_enter_data}}
diff --git a/offload/test/offloading/fortran/target-is-device-ptr.f90 b/offload/test/offloading/fortran/target-is-device-ptr.f90
new file mode 100644
index 0000000000000..6010d59113498
--- /dev/null
+++ b/offload/test/offloading/fortran/target-is-device-ptr.f90
@@ -0,0 +1,79 @@
+! Validate that a device pointer allocated via OpenMP runtime APIs can be
+! consumed by a TARGET region using the is_device_ptr clause.
+! REQUIRES: flang, amdgcn-amd-amdhsa
+! UNSUPPORTED: nvptx64-nvidia-cuda
+! UNSUPPORTED: nvptx64-nvidia-cuda-LTO
+! UNSUPPORTED: aarch64-unknown-linux-gnu
+! UNSUPPORTED: aarch64-unknown-linux-gnu-LTO
+! UNSUPPORTED: x86_64-unknown-linux-gnu
+! UNSUPPORTED: x86_64-unknown-linux-gnu-LTO
+
+! RUN: %libomptarget-compile-fortran-run-and-check-generic
+
+program is_device_ptr_target
+ use omp_lib
+ use iso_c_binding
+ implicit none
+
+ integer, parameter :: n = 4
+ integer, target :: host(n)
+ type(c_ptr) :: device_ptr
+ integer(c_int) :: rc
+ integer :: i
+
+ do i = 1, n
+ host(i) = i
+ end do
+
+ device_ptr = omp_target_alloc(int(n, c_size_t) * int(c_sizeof(host(1)), c_size_t), &
+ omp_get_default_device())
+ if (.not. c_associated(device_ptr)) then
+ print *, "device alloc failed"
+ stop 1
+ end if
+
+ rc = omp_target_memcpy(device_ptr, c_loc(host), &
+ int(n, c_size_t) * int(c_sizeof(host(1)), c_size_t), &
+ 0_c_size_t, 0_c_size_t, &
+ omp_get_default_device(), omp_get_initial_device())
+ if (rc .ne. 0) then
+ print *, "host->device memcpy failed"
+ call omp_target_free(device_ptr, omp_get_default_device())
+ stop 1
+ end if
+
+ call fill_on_device(device_ptr)
+
+ rc = omp_target_memcpy(c_loc(host), device_ptr, &
+ int(n, c_size_t) * int(c_sizeof(host(1)), c_size_t), &
+ 0_c_size_t, 0_c_size_t, &
+ omp_get_initial_device(), omp_get_default_device())
+ call omp_target_free(device_ptr, omp_get_default_device())
+
+ if (rc .ne. 0) then
+ print *, "device->host memcpy failed"
+ stop 1
+ end if
+
+ if (all(host == [2, 4, 6, 8])) then
+ print *, "PASS"
+ else
+ print *, "FAIL", host
+ end if
+
+contains
+ subroutine fill_on_device(ptr)
+ type(c_ptr) :: ptr
+ integer, pointer :: p(:)
+ call c_f_pointer(ptr, p, [n])
+
+ !$omp target is_device_ptr(ptr)
+ p(1) = 2
+ p(2) = 4
+ p(3) = 6
+ p(4) = 8
+ !$omp end target
+ end subroutine fill_on_device
+end program is_device_ptr_target
+
+!CHECK: PASS
|
ergawy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Akash. Just a few stop-by comments, I hope that's ok with you.
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
Outdated
Show resolved
Hide resolved
Thanks a lot for the comments, they are much appreciated. I've resolved them in the latest revision. |
| call c_f_pointer(ptr, p, [n]) | ||
|
|
||
| !$omp target is_device_ptr(ptr) | ||
| p(1) = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c_f_pointer should be called inside the target region, if the goal is to test whether the argument for ptr was set correctly. The uninitialized p can then be made private on the target construct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tweaked the test overall. Can you please comment if you're happy with the change? Thanks.
| dptr = omp_get_mapped_ptr(c_loc(a), dev) | ||
|
|
||
| !$omp target is_device_ptr(dptr) map(tofrom: flag) | ||
| flag = flag + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dptr is still not used inside the target construct. Is your goal to validate that dptr is correctly passed into the target, or just that this doesn't cause a segfault/offload-failure?
If it's the former, then one way to do it is by calling c_f_pointer inside the target region, and reading/writing the pointee via the fortran pointer. Another way is to transfer dptr into an INTEGER(C_INTPTR_T) both inside and outside the target region, print it, and CHECK that the two addresses printed are identical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried adding the call to c_f_pointer inside the target region in the previous test. But it causes a runtime failure, and I'm not sure if the failure is directly related to the is_device_ptr implementation or some other mapping bug.
If you're happy with the current simpler test for this PR, then I can work on debugging a fix for that failure when I return from vacation in the new year. Otherwise, I'll put this PR on hold until I can come up with a working version of that test with a call to c_f_pointer inside the target region.
Let me know which you prefer, thanks.
ergawy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Akash. Just a few comments about mapping flags. Also, can you please add an MLIR to LLVM test?
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
Outdated
Show resolved
Hide resolved
52a71ea to
f5d82d2
Compare
This PR adds support for the OpenMP is_device_ptr clause in the MLIR to LLVM IR translation for target regions. The is_device_ptr clause allows device pointers (allocated via OpenMP runtime APIs) to be used directly in target regions without implicit mapping.
ec3c562 to
2479918
Compare
Thanks for the review, I've added an MLIR test although it checks the same thing as the flang/test/Integration/OpenMP/map-types-and-sizes.f90 test. |
This PR adds support for the OpenMP
is_device_ptrclause in the MLIR to LLVM IR translation for target regions. Theis_device_ptrclause allows device pointers (allocated via OpenMP runtime APIs) to be used directly in target regions without implicit mapping.