[flang] Changes to map variables in link clause of declare target

[anchuraj]

As per the OpenMP standard, "If a variable appears in a link clause on a declare target directive that does not have a device_type clause with the nohost device-type-description then it is treated as if it had appeared in a map clause with a map-type of tofrom" is an implicit mapping rule. Before this change, such variables were mapped as to by default.

[llvmbot]

@llvm/pr-subscribers-flang-fir-hlfir

@llvm/pr-subscribers-flang-openmp

Author: Anchu Rajendran S (anchu-rajendran)

Changes

As per the OpenMP standard, "If a variable appears in a link clause on a declare target directive that does not have a device_type clause with the nohost device-type-description then it is treated as if it had appeared in a map clause with a map-type of tofrom" is an implicit mapping rule. Before this change, such variables were mapped as to by default.

---

Full diff: https://github.com/llvm/llvm-project/pull/83643.diff

2 Files Affected:

• (modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+15-1)
• (added) flang/test/Lower/OpenMP/declare-target-link-tarop-cap.f90 (+55)

diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index 90fc1f80f57ab7..4d160216077aab 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -1120,7 +1120,21 @@ genTargetOp(Fortran::lower::AbstractConverter &converter,
         if (auto refType = baseOp.getType().dyn_cast<fir::ReferenceType>())
           eleType = refType.getElementType();
 
-        if (fir::isa_trivial(eleType) || fir::isa_char(eleType)) {
+        // If a variable is specified in declare target link and if device
+        // type is nohost, it needs to be mapped tofrom
+        mlir::ModuleOp mod = converter.getFirOpBuilder().getModule();
+        mlir::Operation *op = mod.lookupSymbol(converter.mangleName(sym));
+        auto declareTargetOp =
+            llvm::dyn_cast_if_present<mlir::omp::DeclareTargetInterface>(op);
+        if (declareTargetOp && declareTargetOp.isDeclareTarget()) {
+          if (declareTargetOp.getDeclareTargetCaptureClause() ==
+                  mlir::omp::DeclareTargetCaptureClause::link &&
+              declareTargetOp.getDeclareTargetDeviceType() !=
+                  mlir::omp::DeclareTargetDeviceType::nohost) {
+            mapFlag |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO;
+            mapFlag |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
+          }
+        } else if (fir::isa_trivial(eleType) || fir::isa_char(eleType)) {
           captureKind = mlir::omp::VariableCaptureKind::ByCopy;
         } else if (!fir::isa_builtin_cptr_type(eleType)) {
           mapFlag |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO;
diff --git a/flang/test/Lower/OpenMP/declare-target-link-tarop-cap.f90 b/flang/test/Lower/OpenMP/declare-target-link-tarop-cap.f90
new file mode 100644
index 00000000000000..56f7db4d060d95
--- /dev/null
+++ b/flang/test/Lower/OpenMP/declare-target-link-tarop-cap.f90
@@ -0,0 +1,55 @@
+!RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
+!RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-is-device %s -o - | FileCheck %s
+!RUN: bbc -emit-hlfir -fopenmp %s -o - | FileCheck %s
+!RUN: bbc -emit-hlfir -fopenmp -fopenmp-is-target-device %s -o - | FileCheck %s
+
+program test_link
+
+integer :: test_int = 1
+!$omp declare target link(test_int)
+
+integer :: test_array_1d(3) = (/1,2,3/)
+!$omp declare target link(test_array_1d)
+
+integer, pointer :: test_ptr1
+!$omp declare target link(test_ptr1)
+
+integer, target :: test_target = 1
+!$omp declare target link(test_target)
+
+integer, pointer :: test_ptr2
+!$omp declare target link(test_ptr2)
+
+!CHECK-DAG: {{%.*}} = omp.map_info var_ptr({{%.*}} : !fir.ref<i32>, i32) map_clauses(implicit, tofrom) capture(ByRef) -> !fir.ref<i32> {name = "test_int"}
+!$omp target
+  test_int = test_int + 1
+!$omp end target
+
+
+!CHECK-DAG: {{%.*}} = omp.map_info var_ptr({{%.*}} : !fir.ref<!fir.array<3xi32>>, !fir.array<3xi32>) map_clauses(implicit, tofrom) capture(ByRef) bounds({{%.*}}) -> !fir.ref<!fir.array<3xi32>> {name = "test_array_1d"}
+!$omp target
+  do i = 1,3
+    test_array_1d(i) = i * 2
+  end do
+!$omp end target
+
+allocate(test_ptr1)
+test_ptr1 = 1
+!CHECK-DAG: {{%.*}} = omp.map_info var_ptr({{%.*}} : !fir.ref<!fir.box<!fir.ptr<i32>>>, !fir.box<!fir.ptr<i32>>) map_clauses(implicit, tofrom) capture(ByRef) members({{%.*}} : !fir.llvm_ptr<!fir.ref<i32>>) -> !fir.ref<!fir.box<!fir.ptr<i32>>> {name = "test_ptr1"}
+!$omp target
+  test_ptr1 = test_ptr1 + 1
+!$omp end target
+
+!CHECK-DAG: {{%.*}} = omp.map_info var_ptr({{%.*}} : !fir.ref<i32>, i32) map_clauses(implicit, tofrom) capture(ByRef) -> !fir.ref<i32> {name = "test_target"}
+!$omp target
+  test_target = test_target + 1
+!$omp end target
+
+
+!CHECK-DAG: {{%.*}} = omp.map_info var_ptr({{%.*}} : !fir.ref<!fir.box<!fir.ptr<i32>>>, !fir.box<!fir.ptr<i32>>) map_clauses(implicit, tofrom) capture(ByRef) members({{%.*}} : !fir.llvm_ptr<!fir.ref<i32>>) -> !fir.ref<!fir.box<!fir.ptr<i32>>> {name = "test_ptr2"}
+test_ptr2 => test_target
+!$omp target
+  test_ptr2 = test_ptr2 + 1
+!$omp end target
+
+end

[agozillon]

Great work! However, should this be an == rather than a != or is the comment at line 1123~ misworded (there's also a chance I am misreading something as it's very late on a Friday, so I appologies if that's the case!). Going off of the PR description I'd wager the comment might need a little rewording from "and if device type is nohost" to "and if device type is not nohost"

Otherwise, this PR looks good to me, I'll give it a little test on Monday on the local map tests I have just to be sure (will almost certainly be fine, but I am a little OCD with the tests I suppose!) and await your reply to the above comment before signing off on it fully!

[agozillon]

And as an aside, if it wouldn't be too much of a bother could you add a runtime test to: https://github.com/llvm/llvm-project/tree/main/openmp/libomptarget/test/offloading/fortran not necessary for me signing off, but it would be excellent to track this behavior with a runtime test as well!

[anchuraj]

And as an aside, if it wouldn't be too much of a bother could you add a runtime test to: https://github.com/llvm/llvm-project/tree/main/openmp/libomptarget/test/offloading/fortran not necessary for me signing off, but it would be excellent to track this behavior with a runtime test as well!

Hi @agozillon ,
Thank you for reviewing my PR. My comment is incorrect. Thanks for noticing and for the suggestion of adding a runtime test. I do agree with it. I will raise a second revision with the changes.

[agozillon]

Had a chance to test and it all ran nicely as expected, so the PR LGTM minus the two prior comments (one a minor comment fix I think and the other an optional test addition that I'll leave up to you if you wish to add or not)!

Thank you very much for the fix.

[agozillon]

Thank you for the new test! I think it's good, however, it may have been better to add a test with an implicitly captured scalar that had been marked declare target link, the array would have already been marked with the correct ToFrom marking as it's the default behavior! I think your PR will primarily fix the scalars getting the incorrect mapping when used in conjunction with declare target. But in any case, that's me splitting hairs and I don't mind if you're happy with the test!

[anchuraj]

Thanks @agozillon. Testing on a scalar would be more appropriate as you said , I have added a new test applying link to a scalar.

[agozillon]

Thank you very much for humoring my testing OCD! I am sorry for the bother.

The PR looks in very good shape to me now and I'd be more than happy for it to land, but do feel free to wait on any other approvals/reviews you would like to :-)!

[raghavendhra]

LGTM :)