Skip to content

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

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

Merged
merged 4 commits into from
Mar 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion flang/lib/Lower/OpenMP/OpenMP.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 not specified as `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() !=
Copy link
Contributor

@agozillon agozillon Mar 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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;
Expand Down
55 changes: 55 additions & 0 deletions flang/test/Lower/OpenMP/declare-target-link-tarop-cap.f90
Original file line number Diff line number Diff line change
@@ -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

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
! Offloading test with a target region mapping a declare target
! Fortran array writing some values to it and checking the host
! correctly receives the updates made on the device.
! REQUIRES: flang
! UNSUPPORTED: nvptx64-nvidia-cuda-LTO
! UNSUPPORTED: aarch64-unknown-linux-gnu
! UNSUPPORTED: aarch64-unknown-linux-gnu-LTO
! UNSUPPORTED: x86_64-pc-linux-gnu
! UNSUPPORTED: x86_64-pc-linux-gnu-LTO

! RUN: %libomptarget-compile-fortran-run-and-check-generic
module test_0
implicit none
INTEGER :: arr1(10) = (/0,0,0,0,0,0,0,0,0,0/)
INTEGER :: arr2(10) = (/0,0,0,0,0,0,0,0,0,0/)
!$omp declare target link(arr1) enter(arr2)
INTEGER :: scalar = 1
!$omp declare target link(scalar)
end module test_0

subroutine test_with_array_link_and_tofrom()
use test_0
integer :: i = 1
integer :: j = 11
!$omp target map(tofrom:arr1, i, j)
do while (i <= j)
arr1(i) = i;
i = i + 1
end do
!$omp end target

! CHECK: 1 2 3 4 5 6 7 8 9 10
PRINT *, arr1(:)
end subroutine test_with_array_link_and_tofrom

subroutine test_with_array_link_only()
use test_0
integer :: i = 1
integer :: j = 11
!$omp target map(i, j)
do while (i <= j)
arr1(i) = i + 1;
i = i + 1
end do
!$omp end target

! CHECK: 2 3 4 5 6 7 8 9 10 11
PRINT *, arr1(:)
end subroutine test_with_array_link_only

subroutine test_with_array_enter_only()
use test_0
integer :: i = 1
integer :: j = 11
!$omp target map(i, j)
do while (i <= j)
arr2(i) = i + 1;
i = i + 1
end do
!$omp end target

! CHECK: 0 0 0 0 0 0 0 0 0 0
PRINT *, arr2(:)
end subroutine test_with_array_enter_only

subroutine test_with_scalar_link_only()
use test_0
!$omp target
scalar = 10
!$omp end target

! CHECK: 10
PRINT *, scalar
end subroutine test_with_scalar_link_only

program main
call test_with_array_link_and_tofrom()
call test_with_array_link_only()
call test_with_array_enter_only()
call test_with_scalar_link_only()
end program