[OpenMP][MLIR] Preserve to/from flags in mapper base entry for mappers #159799
Conversation
With declare mapper, the parent base entry was emitted as TARGET_PARAM only. The mapper received a map-type without to/from, causing components to degrade to alloc-only (no copies), breaking allocatable payload mapping. This PR preseves the map-type bits from the parent.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in OpenMP/MLIR where declare mapper statements for derived types with allocatable components were not preserving TO/FROM semantics, causing components to degrade to allocation-only without proper data copying.
- Preserves map-type bits (TO/FROM and modifiers) from parent entry when user-defined mappers are used
- Ensures allocatable payload mapping works correctly by maintaining copy semantics
- Adds comprehensive test coverage for the allocatable component scenario
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| offload/test/offloading/fortran/target-declare-mapper-allocatable.f90 | Adds test case validating declare mapper with allocatable components preserves TO/FROM semantics |
| mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | Modifies mapper base entry logic to preserve parent map-type flags when user-defined mappers are present |
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-openmp Author: Akash Banerjee (TIFitis) ChangesWith declare mapper, the parent base entry was emitted as This fixes #156466. Full diff: https://github.com/llvm/llvm-project/pull/159799.diff 2 Files Affected:
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 5e194dc715d59..4921a1990b6e8 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -4089,11 +4089,30 @@ static llvm::omp::OpenMPOffloadMappingFlags mapParentWithMembers(
assert(!ompBuilder.Config.isTargetDevice() &&
"function only supported for host device codegen");
- // Map the first segment of our structure
- combinedInfo.Types.emplace_back(
+ // Map the first segment of the parent. If a user-defined mapper is attached,
+ // include the parent's to/from-style bits (and common modifiers) in this
+ // base entry so the mapper receives correct copy semantics via its 'type'
+ // parameter. Also keep TARGET_PARAM when required for kernel arguments.
+ llvm::omp::OpenMPOffloadMappingFlags baseFlag =
isTargetParams
? llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TARGET_PARAM
- : llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_NONE);
+ : llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_NONE;
+
+ // Detect if this mapping uses a user-defined mapper.
+ bool hasUserMapper = mapData.Mappers[mapDataIndex] != nullptr;
+ if (hasUserMapper) {
+ using mapFlags = llvm::omp::OpenMPOffloadMappingFlags;
+ // Preserve relevant map-type bits from the parent clause. These include
+ // the copy direction (TO/FROM), as well as commonly used modifiers that
+ // should be visible to the mapper for correct behaviour.
+ mapFlags parentFlags = mapData.Types[mapDataIndex];
+ mapFlags preserve = mapFlags::OMP_MAP_TO | mapFlags::OMP_MAP_FROM |
+ mapFlags::OMP_MAP_ALWAYS | mapFlags::OMP_MAP_CLOSE |
+ mapFlags::OMP_MAP_PRESENT | mapFlags::OMP_MAP_OMPX_HOLD;
+ baseFlag |= (parentFlags & preserve);
+ }
+
+ combinedInfo.Types.emplace_back(baseFlag);
combinedInfo.DevicePointers.emplace_back(
mapData.DevicePointers[mapDataIndex]);
combinedInfo.Mappers.emplace_back(mapData.Mappers[mapDataIndex]);
diff --git a/offload/test/offloading/fortran/target-declare-mapper-allocatable.f90 b/offload/test/offloading/fortran/target-declare-mapper-allocatable.f90
new file mode 100644
index 0000000000000..d8d5e1b5631a5
--- /dev/null
+++ b/offload/test/offloading/fortran/target-declare-mapper-allocatable.f90
@@ -0,0 +1,48 @@
+! This test validates that declare mapper for a derived type with an
+! allocatable component preserves TO/FROM semantics for the component,
+! ensuring the payload is copied back to the host on target exit.
+
+! REQUIRES: flang, amdgpu
+
+! RUN: %libomptarget-compile-fortran-run-and-check-generic
+
+program target_declare_mapper_allocatable
+ implicit none
+
+ type :: real_t
+ real, allocatable :: real_arr(:)
+ end type real_t
+
+ ! Map the allocatable array payload via a named mapper.
+ !$omp declare mapper (xyz : real_t :: t) map(tofrom: t%real_arr)
+
+ type(real_t) :: r
+ integer :: i
+ logical :: ok
+
+ allocate(r%real_arr(10))
+ r%real_arr = 1.0
+
+ !$omp target map(mapper(xyz), tofrom: r)
+ do i = 1, size(r%real_arr)
+ r%real_arr(i) = 3.0
+ end do
+ !$omp end target
+
+ ok = .true.
+ do i = 1, size(r%real_arr)
+ if (r%real_arr(i) /= 3.0) ok = .false.
+ end do
+ if (ok) then
+ print *, "Test passed!"
+ else
+ print *, "Test failed!"
+ do i = 1, size(r%real_arr)
+ print *, r%real_arr(i)
+ end do
+ end if
+
+ deallocate(r%real_arr)
+end program target_declare_mapper_allocatable
+
+! CHECK: Test passed!
|
agozillon
left a comment
agozillon
left a comment
There was a problem hiding this comment.
This looks fine to me, but is there a reason we can't emit the same entries from the frontend as we would if we explicitly mapped map(tofrom: r%real_arr)? As far as I'm aware that mapping should work upstream currently (if it doesn't then it will in the nearish future once the PRs I have up land and I can look into tidying up if at all possible). Just wondering as it'd be nice if we could avoid explicitly specialising the lowering for the mapper, if we can't its fine just curious :-)
Super thanks for the quick review. |
…r mappers (llvm#159799)" This reverts commit 8afea0d.
llvm#159799) With declare mapper, the parent base entry was emitted as `TARGET_PARAM` only. The mapper received a map-type without `to/from`, causing components to degrade to `alloc`-only (no copies), breaking allocatable payload mapping. This PR preserves the map-type bits from the parent. This fixes llvm#156466.
3 participants
With declare mapper, the parent base entry was emitted as
TARGET_PARAMonly. The mapper received a map-type withoutto/from, causing components to degrade toalloc-only (no copies), breaking allocatable payload mapping. This PR preserves the map-type bits from the parent.This fixes #156466.