Skip to content

Changes to support link clause of declare target with common block #84825

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Changes to support link clause of declare target with common block #84825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 26 additions & 5 deletions flang/lib/Lower/OpenMP/OpenMP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1037,6 +1037,31 @@ static void genBodyOfTargetOp(
genNestedEvaluations(converter, eval);
}

// If the symbol is specified in declare target directive, the function returns
// the corresponding declare target operation.
static mlir::omp::DeclareTargetInterface
getDeclareTargetOp(const Fortran::semantics::Symbol &sym,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice PR so far! :-)

However, I have a feeling this is only part of the puzzle with common blocks. I don't know enough about common blocks and OpenMPs interaction with them enough yet though, so I could most definitely be wrong. @mjklemm or @kiranchandramohan may perhaps know better in this case.

However, if you emit the LLVM-IR for the two cases you've added to: openmp/libomptarget/test/offloading/fortran/declare-target-vars-in-target-region.f90

You will see that the variables used in the regions are being treated as regular (implicitly) mapped variables, rather than declare target variables, and that the common block global is actually generated as a declare target global variable but unused on device. The elements of the common block are being accessed via a GEP and then passed to the target region, which seems reasonable from the perspective of infrastructure that's in place currently but I am not sure that's what we'd like in these cases.

I do not know if members of a declare target'd common block themselves should be treated as if they were declare target or not though (this looks from IR inspection to be something that could be quite difficult from the way common blocks are represented). From this specification exert on page 61 of the OpenMP 5.2 specification:

"When a named common block appears in an OpenMP argument list, it has the same meaning and restrictions as if every explicit member of the common block appeared in the list. An explicit member of a common block is a variable that is named in a COMMON statement that specifies the common block name and is declared in the same scoping unit in which the clause appears. Named common blocks do not include the blank common block"

It seems like that may be the case, but my OpenMP-fu is trainee level at best, so please do verify for yourself and then get a second opinion!

I think referring to a common block symbol directly in a map clause e.g. map(tofrom: /CB/) might be another good initial test if it is legal. From what I can tell it is, based on the definition of the map clause and locator lists (the latter also on page 61) but please do verify for yourself and if in doubt ask someone internally (or externally) who has more OpenMP specification knowledge than I do! The initial issue you will likely face with that test is that I do not believe it will go through the parser as I think it will complain about the symbol, I think it needs taught to deal with it like declare target currently does...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @agozillon for reviewing my PR. For the scalar, it seems to refer the common block global. However, for arrays it does not seem to. Let me dive a little deeper and will update this thread with my findings

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you very much @anchuraj please do take the concerns I stated above with a grain of salt and feel free to tell me if I am wrong (as there's a good chance I am), it's thoughts from my previous work on declare target which didn't touch on common blocks other than to know it's something that OpenMP seems to require handling of. So at the very least it's something we can learn about together as we go :-)

Copy link
Contributor

@agozillon agozillon Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think the main thing to keep in mind is that variablename_ref_ptr becomes the declare target variable rather than the original for the device IR and it's the thing that likely should be utilised to access the underlying data. This generated ref pointer is also utilised as the base pointer for the kernel entry info for the declare target argument as well in the host IR. This seems to only be relevant for the link clause however.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @agozillon . References to common block structure in llvm ir is not correct. It is not appropriately looking at the right index even for scalar. As you said, we will have to update the code generation to refer to the right index in the common block global.

1 PROGRAM TEST
 2
 3 INTEGER A
 4 INTEGER CBA
 5 COMMON /C1/ A, CBA
 6
 7 !$OMP DECLARE TARGET LINK(/C1/)
 8
 9 A=2
10 CBA=100
11 !$OMP TARGET
12 A=A+1
13 CBA=CBA+1
14 !$OMP END TARGET
15 PRINT *, CBA
16 END

The above code prints wrong result in case of scalars as indexing is not done properly.
I am cancelling this PR and will try to solve this problem .

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for looking into this @anchuraj, I think this might be a bit of a hard one to solve, but I've been wrong before (on a few occasions, and it's always a pleasant surprise). Do let me know if there is anything I can help with and I'll do my best, although, there is most definitely people that are more knowledgeable when it comes to common blocks.

Fortran::lower::AbstractConverter &converter) {
mlir::ModuleOp mod = converter.getFirOpBuilder().getModule();
mlir::Operation *op;
op = mod.lookupSymbol(converter.mangleName(sym));
auto declareTargetOp =
llvm::dyn_cast_if_present<mlir::omp::DeclareTargetInterface>(op);
// If declare target op is not found Check if common block containing the
// variable is specified in declare target
if (!declareTargetOp || !declareTargetOp.isDeclareTarget()) {
if (auto cB = Fortran::semantics::FindCommonBlockContaining(sym)) {
op = mod.lookupSymbol(converter.mangleName(*cB));
declareTargetOp =
llvm::dyn_cast_if_present<mlir::omp::DeclareTargetInterface>(op);
}
}
if (declareTargetOp && declareTargetOp.isDeclareTarget()) {
return declareTargetOp;
}
return static_cast<mlir::omp::DeclareTargetInterface>(nullptr);
}

static mlir::omp::TargetOp
genTargetOp(Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
Expand Down Expand Up @@ -1122,11 +1147,7 @@ genTargetOp(Fortran::lower::AbstractConverter &converter,

// 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 (auto declareTargetOp = getDeclareTargetOp(sym, converter)) {
if (declareTargetOp.getDeclareTargetCaptureClause() ==
mlir::omp::DeclareTargetCaptureClause::link &&
declareTargetOp.getDeclareTargetDeviceType() !=
Expand Down
18 changes: 18 additions & 0 deletions flang/test/Lower/OpenMP/declare-target-link-tarop-cap.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@ program test_link
integer, pointer :: test_ptr2
!$omp declare target link(test_ptr2)

integer :: test_int_cb

integer :: test_int_array_cb(3) = (/1,2,3/)

common /test_cb/ test_int_cb, test_int_array_cb
!$omp declare target link(/test_cb/)

!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
Expand Down Expand Up @@ -52,4 +59,15 @@ program test_link
test_ptr2 = test_ptr2 + 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_int_cb"}
!$omp target
test_int_cb = test_int_cb + 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_int_array_cb"}
!$omp target
do i = 1,3
test_int_array_cb(i) = i * 2
end do
!$omp end target
end
31 changes: 31 additions & 0 deletions openmp/libomptarget/test/offloading/fortran/declare-target-vars-in-target-region.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ module test_0
!$omp declare target link(arr1) enter(arr2)
INTEGER :: scalar = 1
!$omp declare target link(scalar)
INTEGER :: scalar_cb = 1
INTEGER :: arr_cb(10) = (/0,0,0,0,0,0,0,0,0,0/)
COMMON /CB/ scalar_cb, arr_cb
!$omp declare target link(/CB/)
end module test_0

subroutine test_with_array_link_and_tofrom()
Expand Down Expand Up @@ -73,9 +77,36 @@ subroutine test_with_scalar_link_only()
PRINT *, scalar
end subroutine test_with_scalar_link_only

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

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

subroutine test_with_scalar_cb_link_only()
use test_0
!$omp target
scalar_cb = 10
!$omp end target

! CHECK: 10
PRINT *, scalar_cb
end subroutine test_with_scalar_cb_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()
call test_with_array_cb_link_only()
call test_with_scalar_cb_link_only()
end program