[flang][OpenMP] Support custom mappers in target update to/from clauses #169673
+196
−87
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
flang
Member
|
@llvm/pr-subscribers-flang-semantics @llvm/pr-subscribers-flang-openmp Author: Krish Gupta (KrxGu) ChangesFixes #168701 This patch adds support for custom mappers in OpenMP Previously, custom mappers were only supported in
Testing:
Full diff: https://github.com/llvm/llvm-project/pull/169673.diff 3 Files Affected:
diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp
index 2a487a6d39d51..ca17767dce924 100644
--- a/flang/lib/Semantics/resolve-names.cpp
+++ b/flang/lib/Semantics/resolve-names.cpp
@@ -1631,6 +1631,8 @@ class OmpVisitor : public virtual DeclarationVisitor {
return true;
}
bool Pre(const parser::OmpMapClause &);
+ bool Pre(const parser::OmpClause::To &);
+ bool Pre(const parser::OmpClause::From &);
bool Pre(const parser::OpenMPSectionsConstruct &) {
PushScope(Scope::Kind::OtherConstruct, nullptr);
@@ -1876,6 +1878,64 @@ bool OmpVisitor::Pre(const parser::OmpMapClause &x) {
return true;
}
+bool OmpVisitor::Pre(const parser::OmpClause::To &x) {
+ // Resolve mapper names in "to" clauses (e.g., for target update)
+ auto &mods{OmpGetModifiers(x.v)};
+ if (auto *mapper{OmpGetUniqueModifier<parser::OmpMapper>(mods)}) {
+ if (auto *symbol{FindSymbol(currScope(), mapper->v)}) {
+ auto &ultimate{symbol->GetUltimate()};
+ auto *misc{ultimate.detailsIf<MiscDetails>()};
+ auto *md{ultimate.detailsIf<MapperDetails>()};
+ if (!md && (!misc || misc->kind() != MiscDetails::Kind::ConstructName))
+ context().Say(mapper->v.source,
+ "Name '%s' should be a mapper name"_err_en_US, mapper->v.source);
+ else
+ mapper->v.symbol = symbol;
+ } else {
+ // Allow the special 'default' mapper identifier without prior
+ // declaration so lowering can recognize and handle it. Emit an
+ // error for any other missing mapper identifier.
+ if (mapper->v.source.ToString() == "default") {
+ mapper->v.symbol = &MakeSymbol(
+ mapper->v, MiscDetails{MiscDetails::Kind::ConstructName});
+ } else {
+ context().Say(
+ mapper->v.source, "'%s' not declared"_err_en_US, mapper->v.source);
+ }
+ }
+ }
+ return true;
+}
+
+bool OmpVisitor::Pre(const parser::OmpClause::From &x) {
+ // Resolve mapper names in "from" clauses (e.g., for target update)
+ auto &mods{OmpGetModifiers(x.v)};
+ if (auto *mapper{OmpGetUniqueModifier<parser::OmpMapper>(mods)}) {
+ if (auto *symbol{FindSymbol(currScope(), mapper->v)}) {
+ auto &ultimate{symbol->GetUltimate()};
+ auto *misc{ultimate.detailsIf<MiscDetails>()};
+ auto *md{ultimate.detailsIf<MapperDetails>()};
+ if (!md && (!misc || misc->kind() != MiscDetails::Kind::ConstructName))
+ context().Say(mapper->v.source,
+ "Name '%s' should be a mapper name"_err_en_US, mapper->v.source);
+ else
+ mapper->v.symbol = symbol;
+ } else {
+ // Allow the special 'default' mapper identifier without prior
+ // declaration so lowering can recognize and handle it. Emit an
+ // error for any other missing mapper identifier.
+ if (mapper->v.source.ToString() == "default") {
+ mapper->v.symbol = &MakeSymbol(
+ mapper->v, MiscDetails{MiscDetails::Kind::ConstructName});
+ } else {
+ context().Say(
+ mapper->v.source, "'%s' not declared"_err_en_US, mapper->v.source);
+ }
+ }
+ }
+ return true;
+}
+
void OmpVisitor::ProcessMapperSpecifier(const parser::OmpMapperSpecifier &spec,
const parser::OmpClauseList &clauses) {
// This "manually" walks the tree of the construct, because we need
diff --git a/flang/test/Lower/OpenMP/target-update-mapper.f90 b/flang/test/Lower/OpenMP/target-update-mapper.f90
new file mode 100644
index 0000000000000..d49ad51343679
--- /dev/null
+++ b/flang/test/Lower/OpenMP/target-update-mapper.f90
@@ -0,0 +1,52 @@
+! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=52 %s -o - | FileCheck %s
+
+! Test mapper usage in target update to/from clauses
+
+program target_update_mapper
+ implicit none
+
+ integer, parameter :: n = 4
+
+ type :: typ
+ integer, allocatable :: a(:)
+ integer, allocatable :: b(:)
+ end type typ
+
+ !$omp declare mapper(custom: typ :: t) map(t%a)
+
+ ! CHECK-LABEL: omp.declare_mapper @_QQFcustom : !fir.type<_QFTtyp{a:!fir.box<!fir.heap<!fir.array<?xi32>>>,b:!fir.box<!fir.heap<!fir.array<?xi32>>>}>
+
+ type(typ) :: t
+
+ allocate(t%a(n), source=1)
+ allocate(t%b(n), source=2)
+
+ !$omp target enter data map(alloc: t)
+
+ ! Test target update to with custom mapper
+ ! CHECK: %[[T_VAR:.*]] = fir.declare %{{.*}} {uniq_name = "_QFtarget_update_mapperEt"} : (!fir.ref<!fir.type<_QFTtyp{a:!fir.box<!fir.heap<!fir.array<?xi32>>>,b:!fir.box<!fir.heap<!fir.array<?xi32>>>}>>) -> !fir.ref<!fir.type<_QFTtyp{a:!fir.box<!fir.heap<!fir.array<?xi32>>>,b:!fir.box<!fir.heap<!fir.array<?xi32>>>}>>
+ ! CHECK: %[[MAP_INFO:.*]] = omp.map.info var_ptr(%[[T_VAR]] : {{.*}}, {{.*}}) map_clauses(to) capture(ByRef) mapper(@_QQFcustom) -> {{.*}}
+ ! CHECK: omp.target_update motion_entries(%[[MAP_INFO]] : {{.*}})
+ t%a = 42
+ !$omp target update to(mapper(custom): t)
+
+ !$omp target
+ t%a(:) = t%a(:) / 2
+ t%b(:) = -1
+ !$omp end target
+
+ ! Test target update from with custom mapper
+ ! CHECK: %[[MAP_INFO2:.*]] = omp.map.info var_ptr(%{{.*}} : {{.*}}, {{.*}}) map_clauses(from) capture(ByRef) mapper(@_QQFcustom) -> {{.*}}
+ ! CHECK: omp.target_update motion_entries(%[[MAP_INFO2]] : {{.*}})
+ !$omp target update from(mapper(custom): t)
+
+ ! Test target update to with default mapper
+ ! CHECK: %[[MAP_INFO3:.*]] = omp.map.info var_ptr(%{{.*}} : {{.*}}, {{.*}}) map_clauses(to) capture(ByRef) mapper(@_QQFtyp_omp_default_mapper) -> {{.*}}
+ ! CHECK: omp.target_update motion_entries(%[[MAP_INFO3]] : {{.*}})
+ !$omp target update to(mapper(default): t)
+
+ !$omp target exit data map(delete: t)
+ deallocate(t%a)
+ deallocate(t%b)
+
+end program target_update_mapper
diff --git a/flang/test/Semantics/OpenMP/target-update-mapper.f90 b/flang/test/Semantics/OpenMP/target-update-mapper.f90
new file mode 100644
index 0000000000000..f03496e155568
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/target-update-mapper.f90
@@ -0,0 +1,54 @@
+! RUN: %python %S/../test_errors.py %s %flang -fopenmp -fopenmp-version=52
+
+! Test mapper usage in target update to/from clauses
+
+program target_update_mapper
+ implicit none
+
+ integer, parameter :: n = 4
+
+ type :: typ
+ integer, allocatable :: a(:)
+ integer, allocatable :: b(:)
+ end type typ
+
+ !$omp declare mapper(custom: typ :: t) map(t%a)
+
+ type(typ) :: t
+ integer :: not_a_mapper
+ allocate(t%a(n), source=1)
+ allocate(t%b(n), source=2)
+
+ !$omp target enter data map(alloc: t)
+
+ ! Valid: using custom mapper with target update to
+ t%a = 42
+ !$omp target update to(mapper(custom): t)
+
+ !$omp target
+ t%a(:) = t%a(:) / 2
+ t%b(:) = -1
+ !$omp end target
+
+ ! Valid: using custom mapper with target update from
+ !$omp target update from(mapper(custom): t)
+
+ ! Valid: using default mapper explicitly
+ !$omp target update to(mapper(default): t)
+
+ print*, t%a
+ print*, t%b
+
+ !$omp target exit data map(delete: t)
+ deallocate(t%a)
+ deallocate(t%b)
+
+ ! Test error case: undefined mapper
+ !ERROR: 'undefined_mapper' not declared
+ !$omp target update to(mapper(undefined_mapper): t)
+
+ ! Test error case: wrong kind of symbol
+ !ERROR: Name 'not_a_mapper' should be a mapper name
+ !$omp target update from(mapper(not_a_mapper): t)
+
+end program target_update_mapper
|
Member
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Krish Gupta (KrxGu) ChangesFixes #168701 This patch adds support for custom mappers in OpenMP Previously, custom mappers were only supported in
Testing:
Full diff: https://github.com/llvm/llvm-project/pull/169673.diff 3 Files Affected:
diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp
index 2a487a6d39d51..ca17767dce924 100644
--- a/flang/lib/Semantics/resolve-names.cpp
+++ b/flang/lib/Semantics/resolve-names.cpp
@@ -1631,6 +1631,8 @@ class OmpVisitor : public virtual DeclarationVisitor {
return true;
}
bool Pre(const parser::OmpMapClause &);
+ bool Pre(const parser::OmpClause::To &);
+ bool Pre(const parser::OmpClause::From &);
bool Pre(const parser::OpenMPSectionsConstruct &) {
PushScope(Scope::Kind::OtherConstruct, nullptr);
@@ -1876,6 +1878,64 @@ bool OmpVisitor::Pre(const parser::OmpMapClause &x) {
return true;
}
+bool OmpVisitor::Pre(const parser::OmpClause::To &x) {
+ // Resolve mapper names in "to" clauses (e.g., for target update)
+ auto &mods{OmpGetModifiers(x.v)};
+ if (auto *mapper{OmpGetUniqueModifier<parser::OmpMapper>(mods)}) {
+ if (auto *symbol{FindSymbol(currScope(), mapper->v)}) {
+ auto &ultimate{symbol->GetUltimate()};
+ auto *misc{ultimate.detailsIf<MiscDetails>()};
+ auto *md{ultimate.detailsIf<MapperDetails>()};
+ if (!md && (!misc || misc->kind() != MiscDetails::Kind::ConstructName))
+ context().Say(mapper->v.source,
+ "Name '%s' should be a mapper name"_err_en_US, mapper->v.source);
+ else
+ mapper->v.symbol = symbol;
+ } else {
+ // Allow the special 'default' mapper identifier without prior
+ // declaration so lowering can recognize and handle it. Emit an
+ // error for any other missing mapper identifier.
+ if (mapper->v.source.ToString() == "default") {
+ mapper->v.symbol = &MakeSymbol(
+ mapper->v, MiscDetails{MiscDetails::Kind::ConstructName});
+ } else {
+ context().Say(
+ mapper->v.source, "'%s' not declared"_err_en_US, mapper->v.source);
+ }
+ }
+ }
+ return true;
+}
+
+bool OmpVisitor::Pre(const parser::OmpClause::From &x) {
+ // Resolve mapper names in "from" clauses (e.g., for target update)
+ auto &mods{OmpGetModifiers(x.v)};
+ if (auto *mapper{OmpGetUniqueModifier<parser::OmpMapper>(mods)}) {
+ if (auto *symbol{FindSymbol(currScope(), mapper->v)}) {
+ auto &ultimate{symbol->GetUltimate()};
+ auto *misc{ultimate.detailsIf<MiscDetails>()};
+ auto *md{ultimate.detailsIf<MapperDetails>()};
+ if (!md && (!misc || misc->kind() != MiscDetails::Kind::ConstructName))
+ context().Say(mapper->v.source,
+ "Name '%s' should be a mapper name"_err_en_US, mapper->v.source);
+ else
+ mapper->v.symbol = symbol;
+ } else {
+ // Allow the special 'default' mapper identifier without prior
+ // declaration so lowering can recognize and handle it. Emit an
+ // error for any other missing mapper identifier.
+ if (mapper->v.source.ToString() == "default") {
+ mapper->v.symbol = &MakeSymbol(
+ mapper->v, MiscDetails{MiscDetails::Kind::ConstructName});
+ } else {
+ context().Say(
+ mapper->v.source, "'%s' not declared"_err_en_US, mapper->v.source);
+ }
+ }
+ }
+ return true;
+}
+
void OmpVisitor::ProcessMapperSpecifier(const parser::OmpMapperSpecifier &spec,
const parser::OmpClauseList &clauses) {
// This "manually" walks the tree of the construct, because we need
diff --git a/flang/test/Lower/OpenMP/target-update-mapper.f90 b/flang/test/Lower/OpenMP/target-update-mapper.f90
new file mode 100644
index 0000000000000..d49ad51343679
--- /dev/null
+++ b/flang/test/Lower/OpenMP/target-update-mapper.f90
@@ -0,0 +1,52 @@
+! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=52 %s -o - | FileCheck %s
+
+! Test mapper usage in target update to/from clauses
+
+program target_update_mapper
+ implicit none
+
+ integer, parameter :: n = 4
+
+ type :: typ
+ integer, allocatable :: a(:)
+ integer, allocatable :: b(:)
+ end type typ
+
+ !$omp declare mapper(custom: typ :: t) map(t%a)
+
+ ! CHECK-LABEL: omp.declare_mapper @_QQFcustom : !fir.type<_QFTtyp{a:!fir.box<!fir.heap<!fir.array<?xi32>>>,b:!fir.box<!fir.heap<!fir.array<?xi32>>>}>
+
+ type(typ) :: t
+
+ allocate(t%a(n), source=1)
+ allocate(t%b(n), source=2)
+
+ !$omp target enter data map(alloc: t)
+
+ ! Test target update to with custom mapper
+ ! CHECK: %[[T_VAR:.*]] = fir.declare %{{.*}} {uniq_name = "_QFtarget_update_mapperEt"} : (!fir.ref<!fir.type<_QFTtyp{a:!fir.box<!fir.heap<!fir.array<?xi32>>>,b:!fir.box<!fir.heap<!fir.array<?xi32>>>}>>) -> !fir.ref<!fir.type<_QFTtyp{a:!fir.box<!fir.heap<!fir.array<?xi32>>>,b:!fir.box<!fir.heap<!fir.array<?xi32>>>}>>
+ ! CHECK: %[[MAP_INFO:.*]] = omp.map.info var_ptr(%[[T_VAR]] : {{.*}}, {{.*}}) map_clauses(to) capture(ByRef) mapper(@_QQFcustom) -> {{.*}}
+ ! CHECK: omp.target_update motion_entries(%[[MAP_INFO]] : {{.*}})
+ t%a = 42
+ !$omp target update to(mapper(custom): t)
+
+ !$omp target
+ t%a(:) = t%a(:) / 2
+ t%b(:) = -1
+ !$omp end target
+
+ ! Test target update from with custom mapper
+ ! CHECK: %[[MAP_INFO2:.*]] = omp.map.info var_ptr(%{{.*}} : {{.*}}, {{.*}}) map_clauses(from) capture(ByRef) mapper(@_QQFcustom) -> {{.*}}
+ ! CHECK: omp.target_update motion_entries(%[[MAP_INFO2]] : {{.*}})
+ !$omp target update from(mapper(custom): t)
+
+ ! Test target update to with default mapper
+ ! CHECK: %[[MAP_INFO3:.*]] = omp.map.info var_ptr(%{{.*}} : {{.*}}, {{.*}}) map_clauses(to) capture(ByRef) mapper(@_QQFtyp_omp_default_mapper) -> {{.*}}
+ ! CHECK: omp.target_update motion_entries(%[[MAP_INFO3]] : {{.*}})
+ !$omp target update to(mapper(default): t)
+
+ !$omp target exit data map(delete: t)
+ deallocate(t%a)
+ deallocate(t%b)
+
+end program target_update_mapper
diff --git a/flang/test/Semantics/OpenMP/target-update-mapper.f90 b/flang/test/Semantics/OpenMP/target-update-mapper.f90
new file mode 100644
index 0000000000000..f03496e155568
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/target-update-mapper.f90
@@ -0,0 +1,54 @@
+! RUN: %python %S/../test_errors.py %s %flang -fopenmp -fopenmp-version=52
+
+! Test mapper usage in target update to/from clauses
+
+program target_update_mapper
+ implicit none
+
+ integer, parameter :: n = 4
+
+ type :: typ
+ integer, allocatable :: a(:)
+ integer, allocatable :: b(:)
+ end type typ
+
+ !$omp declare mapper(custom: typ :: t) map(t%a)
+
+ type(typ) :: t
+ integer :: not_a_mapper
+ allocate(t%a(n), source=1)
+ allocate(t%b(n), source=2)
+
+ !$omp target enter data map(alloc: t)
+
+ ! Valid: using custom mapper with target update to
+ t%a = 42
+ !$omp target update to(mapper(custom): t)
+
+ !$omp target
+ t%a(:) = t%a(:) / 2
+ t%b(:) = -1
+ !$omp end target
+
+ ! Valid: using custom mapper with target update from
+ !$omp target update from(mapper(custom): t)
+
+ ! Valid: using default mapper explicitly
+ !$omp target update to(mapper(default): t)
+
+ print*, t%a
+ print*, t%b
+
+ !$omp target exit data map(delete: t)
+ deallocate(t%a)
+ deallocate(t%b)
+
+ ! Test error case: undefined mapper
+ !ERROR: 'undefined_mapper' not declared
+ !$omp target update to(mapper(undefined_mapper): t)
+
+ ! Test error case: wrong kind of symbol
+ !ERROR: Name 'not_a_mapper' should be a mapper name
+ !$omp target update from(mapper(not_a_mapper): t)
+
+end program target_update_mapper
|
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends OpenMP custom mapper support to target update directives' to() and from() clauses, implementing the OpenMP 5.2 specification. Previously, custom mappers were only available in map clauses.
Key Changes:
- Added mapper resolution for
to()andfrom()clauses in target update directives - Introduced semantic validation for undefined mappers and non-mapper symbols
- Added comprehensive test coverage for both semantic analysis and MLIR lowering
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| flang/lib/Semantics/resolve-names.cpp | Adds Pre(OmpClause::To) and Pre(OmpClause::From) handlers to resolve mapper names in target update clauses |
| flang/test/Semantics/OpenMP/target-update-mapper.f90 | Semantic tests verifying custom/default mapper usage and error detection in target update constructs |
| flang/test/Lower/OpenMP/target-update-mapper.f90 | Lowering tests verifying correct MLIR generation with mapper attributes for target update operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Support mapper modifier in processMotionClauses for to/from clauses
TIFitis
reviewed
Nov 27, 2025
Member
TIFitis
left a comment
TIFitis
left a comment
There was a problem hiding this comment.
Thanks for the PR. I've added some review comments below...
Member
There was a problem hiding this comment.
Move the lowering test to flang/test/Lower/OpenMP/declare-mapper.f90.
Also add an offloading test in offload/test/offloading/fortran/.
Contributor
Author
There was a problem hiding this comment.
I can add an offloading test in offload/test/offloading/fortran/target-update-custom-mapper.f90, however I don't have access to GPU hardware(GPU poor 🙃) and can only test on CPU.
I've prepared the test and verified to the best of my capability:
- Correct Fortran syntax (no syntax errors)
- Compiles successfully to MLIR
- Generated IR contains proper
omp.declare_mapperdeclarations for both custom and default mappers omp.target_updateoperations correctly reference the mappers (mapper(@_QQFcustom)andmapper(@_QQFmy_type_omp_default_mapper))
The test follows the same pattern as existing offloading tests like target-custom-mapper.f90. It will require GPU hardware to actually execute (REQUIRES: flang, amdgpu).
Here's the test code:
! Test custom mappers with target update to/from clauses
! REQUIRES: flang, amdgpu
! RUN: %libomptarget-compile-fortran-run-and-check-generic
program target_update_mapper_test
implicit none
integer, parameter :: n = 100
type :: my_type
integer :: a(n)
integer :: b(n)
end type my_type
! Declare custom mapper that only maps field 'a'
!$omp declare mapper(custom : my_type :: t) map(t%a)
! Declare default mapper that maps both fields
!$omp declare mapper(my_type :: t) map(t%a, t%b)
type(my_type) :: obj
integer :: i, sum_a, sum_b
! Initialize data on host
do i = 1, n
obj%a(i) = i
obj%b(i) = i * 2
end do
! Allocate space on device using custom mapper (only maps 'a')
!$omp target enter data map(mapper(custom), alloc: obj)
! Update field 'a' to device using custom mapper
obj%a = 10
!$omp target update to(mapper(custom): obj)
! Read back field 'a' from device (should be 10)
obj%a = 0
!$omp target update from(mapper(custom): obj)
sum_a = sum(obj%a)
sum_b = sum(obj%b)
! CHECK: Sum of a after update from device: 1000
print *, "Sum of a after update from device:", sum_a
! Field 'b' was never mapped, so should still have original values
! CHECK: Sum of b (never mapped): 10100
print *, "Sum of b (never mapped):", sum_b
! Now test with default mapper (maps both fields)
obj%a = 20
obj%b = 30
!$omp target update to(mapper(default): obj)
obj%a = 0
obj%b = 0
!$omp target update from(mapper(default): obj)
sum_a = sum(obj%a)
sum_b = sum(obj%b)
! CHECK: Sum of a with default mapper: 2000
print *, "Sum of a with default mapper:", sum_a
! CHECK: Sum of b with default mapper: 3000
print *, "Sum of b with default mapper:", sum_b
!$omp target exit data map(delete: obj)
! CHECK: Test passed!
print *, "Test passed!"
end program target_update_mapper_testLet me know if you'd like me to include this in the PR, or if you prefer to validate it on GPU hardware first.
Member
There was a problem hiding this comment.
# .---command stderr------------
# | /home/akash/Documents/llvm-work/offload/test/offloading/fortran/target-declare-mapper-to-from.f90:68:11: error: CHECK: expected string not found in input
# | ! CHECK: Sum of b with default mapper: 3000
# | ^
# | <stdin>:3:36: note: scanning from here
# | Sum of a with default mapper: 2000
# | ^
# | <stdin>:4:2: note: possible intended match here
# | Sum of b with default mapper: 0
# | ^
# |
# | Input file: <stdin>
# | Check file: /home/akash/Documents/llvm-work/offload/test/offloading/fortran/target-declare-mapper-to-from.f90
# |
# | -dump-input=help explains the following input dump.
# |
# | Input was:
# | <<<<<<
# | 1: Sum of a after update from device: 1000
# | 2: Sum of b (never mapped): 10100
# | 3: Sum of a with default mapper: 2000
# | check:68'0 X error: no match found
# | 4: Sum of b with default mapper: 0
# | check:68'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | check:68'1 ? possible intended match
# | 5: Test passed!
# | check:68'0 ~~~~~~~~~~~~~~
# | >>>>>>
# `-----------------------------
This test does not pass on AMDGPU in it's current state c620237
Contributor
Author
There was a problem hiding this comment.
@TIFitis Thanks for testing this! You're absolutely right - I found the logic flaw in my test.
The issue is that I'm mixing mappers inconsistently:
- Line 37: I allocate device memory using
custommapper (which only maps fielda) - Line 58: I try to update using
defaultmapper (which maps bothaandb)
Since field b was never allocated on the device, the update fails and reads back as 0.
Here's a corrected version that properly separates the two test scenarios:
! Test custom mappers with target update to/from clauses
! REQUIRES: flang, amdgpu
! RUN: %libomptarget-compile-fortran-run-and-check-generic
program target_update_mapper_test
implicit none
integer, parameter :: n = 100
type :: my_type
integer :: a(n)
integer :: b(n)
end type my_type
! Declare custom mapper that only maps field 'a'
!$omp declare mapper(custom : my_type :: t) map(t%a)
! Declare default mapper that maps both fields
!$omp declare mapper(my_type :: t) map(t%a, t%b)
type(my_type) :: obj
integer :: i, sum_a, sum_b
! ========== Test 1: Custom mapper (field 'a' only) ==========
! Initialize data on host
do i = 1, n
obj%a(i) = i
obj%b(i) = i * 2
end do
! Allocate and update using custom mapper (only 'a')
!$omp target enter data map(mapper(custom), alloc: obj)
obj%a = 10
!$omp target update to(mapper(custom): obj)
obj%a = 0
!$omp target update from(mapper(custom): obj)
sum_a = sum(obj%a)
sum_b = sum(obj%b)
! CHECK: Sum of a (custom mapper): 1000
print *, "Sum of a (custom mapper):", sum_a
! Field 'b' was never mapped with custom mapper
! CHECK: Sum of b (never mapped): 10100
print *, "Sum of b (never mapped):", sum_b
!$omp target exit data map(mapper(custom), delete: obj)
! ========== Test 2: Default mapper (both fields) ==========
! Re-initialize
do i = 1, n
obj%a(i) = 20
obj%b(i) = 30
end do
! Allocate and update using default mapper (both 'a' and 'b')
!$omp target enter data map(mapper(default), alloc: obj)
!$omp target update to(mapper(default): obj)
obj%a = 0
obj%b = 0
!$omp target update from(mapper(default): obj)
sum_a = sum(obj%a)
sum_b = sum(obj%b)
! CHECK: Sum of a (default mapper): 2000
print *, "Sum of a (default mapper):", sum_a
! CHECK: Sum of b (default mapper): 3000
print *, "Sum of b (default mapper):", sum_b
!$omp target exit data map(mapper(default), delete: obj)
! CHECK: Test passed!
print *, "Test passed!"
end program target_update_mapper_testWould you prefer:
Option 1: I add this corrected test to the PR now (you can validate on GPU hardware through CI)
Option 2: We handle this as a follow-up PR or maybe someone who has GPU access can add it .🙃
Let me know what works best for you!
Member
There was a problem hiding this comment.
Test is still not passing
flang -fopenmp -fopenmp-version=50 -fopenmp-targets=amdgcn-amd-amdhsa --offload-arch=gfx1100 -O3 test.f90 -o a.out && ./a.out
warning: OpenMP support for version 50 in flang is still incomplete
warning: OpenMP support for version 50 in flang is still incomplete
warning: OpenMP support for version 50 in flang is still incomplete
Sum of a (custom mapper): 1000
Sum of b (never mapped): 10100
omptarget message: explicit extension not allowed: host address specified is 0x000064cc7ba9a660 (800 bytes), but device allocation maps to host at 0x000064cc7ba9a660 (400 bytes)
omptarget error: Call to getTargetPointer returned null pointer (device failure or illegal mapping).
omptarget error: Call to targetDataBegin via targetDataMapper for custom mapper failed.
omptarget error: Consult https://openmp.llvm.org/design/Runtimes.html for debugging options.
omptarget error: Source location information not present. Compile with -g or -gline-tables-only.
omptarget fatal error 1: failure of target construct while offloading is mandatory
Aborted (core dumped)
We can't land a patch without a test, even more so when the test is failing.
Unfortunately, I'm going on vacation until next year. Hopefully you can find someone else who can help you with the offloading test.
Cheers
Contributor
Author
There was a problem hiding this comment.
Test is still not passing
flang -fopenmp -fopenmp-version=50 -fopenmp-targets=amdgcn-amd-amdhsa --offload-arch=gfx1100 -O3 test.f90 -o a.out && ./a.out warning: OpenMP support for version 50 in flang is still incomplete warning: OpenMP support for version 50 in flang is still incomplete warning: OpenMP support for version 50 in flang is still incomplete Sum of a (custom mapper): 1000 Sum of b (never mapped): 10100 omptarget message: explicit extension not allowed: host address specified is 0x000064cc7ba9a660 (800 bytes), but device allocation maps to host at 0x000064cc7ba9a660 (400 bytes) omptarget error: Call to getTargetPointer returned null pointer (device failure or illegal mapping). omptarget error: Call to targetDataBegin via targetDataMapper for custom mapper failed. omptarget error: Consult https://openmp.llvm.org/design/Runtimes.html for debugging options. omptarget error: Source location information not present. Compile with -g or -gline-tables-only. omptarget fatal error 1: failure of target construct while offloading is mandatory Aborted (core dumped)We can't land a patch without a test, even more so when the test is failing.
Unfortunately, I'm going on vacation until next year. Hopefully you can find someone else who can help you with the offloading test.
Cheers
@tblah , @kparzysz can someone please help me with this, I really want this PR to get merged.
Extract ResolveMapperModifier helper and simplify tests
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Contributor
Author
|
@TIFitis @kparzysz.
Also fixed the test expectations - they were using incorrect mangled names for program-scoped mappers. CI all green, Good To Merge!! |
TIFitis
reviewed
Dec 4, 2025
KrxGu
force-pushed
the
fix-issue-168701-target-update-mapper
branch
from
a770cf2 to
c620237
Compare
Contributor
Author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #168701
This patch adds support for custom mappers in OpenMP
target updateto()andfrom()clauses, implementing the OpenMP 5.2 specification.Previously, custom mappers were only supported in
mapclauses. This change extends mapper resolution totarget updateconstructs by:Pre(OmpClause::To)andPre(OmpClause::From)handlers inresolve-names.cppOmpMapClausehandlerdefaultmapperTesting: