Changes to support link clause of declare target with common block #84825
Conversation
This change is a continuation to changes in llvm#83643. As per the implicit mapping rules, if a variable is specified in link clause of openmp, it needs to be mapped tofrom if the device type is not specified as nohost.
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-openmp @llvm/pr-subscribers-flang-fir-hlfir Author: Anchu Rajendran S (anchuraj) ChangesThis change is a continuation to changes in #83643. As per the implicit mapping rules, if a variable is specified in link clause of openmp, it needs to be mapped tofrom if the device type is not specified as nohost. Full diff: https://github.com/llvm/llvm-project/pull/84825.diff 3 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index 5cff95c7d125b0..83fb2d94508c26 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -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,
+ 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,
@@ -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() !=
diff --git a/flang/test/Lower/OpenMP/declare-target-link-tarop-cap.f90 b/flang/test/Lower/OpenMP/declare-target-link-tarop-cap.f90
index 7cd0597161578d..cd2615faba5463 100644
--- a/flang/test/Lower/OpenMP/declare-target-link-tarop-cap.f90
+++ b/flang/test/Lower/OpenMP/declare-target-link-tarop-cap.f90
@@ -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
@@ -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
diff --git a/openmp/libomptarget/test/offloading/fortran/declare-target-vars-in-target-region.f90 b/openmp/libomptarget/test/offloading/fortran/declare-target-vars-in-target-region.f90
index f524deac3bcce9..63343d504323b4 100644
--- a/openmp/libomptarget/test/offloading/fortran/declare-target-vars-in-target-region.f90
+++ b/openmp/libomptarget/test/offloading/fortran/declare-target-vars-in-target-region.f90
@@ -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()
@@ -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
|
| // 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, |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 .
There was a problem hiding this comment.
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.
|
The implementation does not solve the problem. Common block has a global created and references in target need to be updated to refer the global. Right now, for scalar, though llvm ir refers to the global, indexing is done incorrect. For arrays, it does not refer to the common block global. Implementation should take into account these issues and solve the problem of correctly emitting ir when common blocks are specified inside declare target. |
This change is a continuation to changes in #83643. As per the implicit mapping rules, if a variable is specified in link clause of openmp, it needs to be mapped tofrom if the device type is not specified as nohost.