From fba0828a37626ed9b4f7af1f51213a1b77962fe7 Mon Sep 17 00:00:00 2001 From: Abid Qadeer Date: Fri, 19 Jan 2024 12:42:36 -0600 Subject: [PATCH 1/2] Generate implicit mapping for reduction variable. The result of the reduction was not being copied back to reduction variable on the host. To sidestep this problem, an explicit map(tofrom: var) has to be added in the code to get the result back. This patch tries to solve this problem by generating an implicit tofrom mapping for reduction variable. All variables used in the target region (and without an explicit mapping) gets an implcit mapping in getTargetOP. We switch on TO,FROM bits for reduction varibles on this mapping. --- flang/lib/Lower/OpenMP.cpp | 26 ++++++++++- flang/test/Lower/OpenMP/reduction_var_map.f90 | 43 +++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) create mode 100644 flang/test/Lower/OpenMP/reduction_var_map.f90 diff --git a/flang/lib/Lower/OpenMP.cpp b/flang/lib/Lower/OpenMP.cpp index 15bad1ca18991..257aa0bf27559 100644 --- a/flang/lib/Lower/OpenMP.cpp +++ b/flang/lib/Lower/OpenMP.cpp @@ -3153,6 +3153,7 @@ genTargetOp(Fortran::lower::AbstractConverter &converter, llvm::SmallVector mapSymTypes; llvm::SmallVector mapSymLocs; llvm::SmallVector mapSymbols; + llvm::SmallVector redSymbols; ClauseProcessor cp(converter, clauseList); cp.processDevice(stmtCtx, deviceOperand); @@ -3178,6 +3179,24 @@ genTargetOp(Fortran::lower::AbstractConverter &converter, .getIsTargetDevice()) cp.processNowait(nowaitAttr); + if (outerCombined) { + for (const Fortran::parser::OmpClause &clause : clauseList.v) { + if (const auto &reductionClause = + std::get_if(&clause.u)) { + const auto &objectList{ + std::get(reductionClause->v.t)}; + + for (const Fortran::parser::OmpObject &ompObject : objectList.v) { + if (const auto *name{ + Fortran::parser::Unwrap(ompObject)}) { + if (const Fortran::semantics::Symbol * symbol{name->symbol}) + redSymbols.push_back(symbol); + } + } + } + } + } + // 5.8.1 Implicit Data-Mapping Attribute Rules // The following code follows the implicit data-mapping rules to map all the // symbols used inside the region that have not been explicitly mapped using @@ -3222,7 +3241,12 @@ genTargetOp(Fortran::lower::AbstractConverter &converter, if (auto refType = baseOp.getType().dyn_cast()) eleType = refType.getElementType(); - if (fir::isa_trivial(eleType) || fir::isa_char(eleType)) { + // Do a tofrom map for reduction variables. + if (llvm::find(redSymbols, &sym) != redSymbols.end()) { + mapFlag |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM; + mapFlag |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO; + captureKind = mlir::omp::VariableCaptureKind::ByRef; + } 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/reduction_var_map.f90 b/flang/test/Lower/OpenMP/reduction_var_map.f90 new file mode 100644 index 0000000000000..edca8dfd903d6 --- /dev/null +++ b/flang/test/Lower/OpenMP/reduction_var_map.f90 @@ -0,0 +1,43 @@ +!RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s + +! This test checks that if reduction clause is on a combined target +! construct, there is an implicit map(tofrom) for each reduction variable. + +! construct with target +subroutine omp_target_combined + implicit none + integer(kind = 8) :: s1 + integer(kind = 8) :: s2 + integer(kind = 4) :: i + s1 = 1 + s2 = 1 + !$omp target teams distribute parallel do reduction(+:s1) reduction(+:s2) + do i=1,1000 + s1 = s1 + i + s2 = s2 + i + end do + !$omp end target teams distribute parallel do + return +end subroutine omp_target_combined +!CHECK-LABEL: func.func @_QPomp_target_combined() { +!CHECK: omp.map_info var_ptr({{.*}} : !fir.ref, i64) map_clauses(implicit, tofrom) capture(ByRef) -> !fir.ref {name = "s1"} +!CHECK: omp.map_info var_ptr({{.*}} : !fir.ref, i64) map_clauses(implicit, tofrom) capture(ByRef) -> !fir.ref {name = "s2"} +!CHECK: omp.map_info var_ptr({{.*}} : !fir.ref, i32) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !fir.ref {name = "i"} + +subroutine omp_target_team_separate + implicit none + integer(kind = 8) :: s3 + integer i + s3 = 1 + !$omp target + s3 = 2 + !$omp teams distribute parallel do reduction(+:s3) + do i=1,1000 + s3 = s3 + i + end do + !$omp end teams distribute parallel do + !$omp end target + return +end subroutine omp_target_team_separate +!CHECK-LABEL: func.func @_QPomp_target_team_separate() { +!CHECK: omp.map_info var_ptr({{.*}} : !fir.ref, i64) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !fir.ref {name = "s3"} From 990c36b337e910d5b54d3d3d122d1a3b059155cc Mon Sep 17 00:00:00 2001 From: Abid Qadeer Date: Tue, 23 Jan 2024 10:19:37 -0600 Subject: [PATCH 2/2] Move code to get the reduction variables into separate function. --- flang/lib/Lower/OpenMP.cpp | 53 ++++++++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 20 deletions(-) diff --git a/flang/lib/Lower/OpenMP.cpp b/flang/lib/Lower/OpenMP.cpp index 257aa0bf27559..a25f5863d43af 100644 --- a/flang/lib/Lower/OpenMP.cpp +++ b/flang/lib/Lower/OpenMP.cpp @@ -643,6 +643,9 @@ class ClauseProcessor { llvm::SmallVectorImpl *mapSymLocs = nullptr, llvm::SmallVectorImpl *mapSymbols = nullptr) const; + bool processTargetReduction( + llvm::SmallVector &reductionSymbols) + const; bool processReduction( mlir::Location currentLocation, llvm::SmallVectorImpl &reductionVars, @@ -1095,6 +1098,21 @@ class ReductionProcessor { return decl; } + static void addReductionSym( + const Fortran::parser::OmpReductionClause &reduction, + llvm::SmallVector &Symbols) { + const auto &objectList{ + std::get(reduction.t)}; + + for (const Fortran::parser::OmpObject &ompObject : objectList.v) { + if (const auto *name{ + Fortran::parser::Unwrap(ompObject)}) { + if (const Fortran::semantics::Symbol * symbol{name->symbol}) + Symbols.push_back(symbol); + } + } + } + /// Creates a reduction declaration and associates it with an OpenMP block /// directive. static void addReductionDecl( @@ -2011,6 +2029,17 @@ bool ClauseProcessor::processMap( }); } +bool ClauseProcessor::processTargetReduction( + llvm::SmallVector &reductionSymbols) + const { + return findRepeatableClause( + [&](const ClauseTy::Reduction *reductionClause, + const Fortran::parser::CharBlock &) { + ReductionProcessor rp; + rp.addReductionSym(reductionClause->v, reductionSymbols); + }); +} + bool ClauseProcessor::processReduction( mlir::Location currentLocation, llvm::SmallVectorImpl &reductionVars, @@ -3153,7 +3182,7 @@ genTargetOp(Fortran::lower::AbstractConverter &converter, llvm::SmallVector mapSymTypes; llvm::SmallVector mapSymLocs; llvm::SmallVector mapSymbols; - llvm::SmallVector redSymbols; + llvm::SmallVector reductionSymbols; ClauseProcessor cp(converter, clauseList); cp.processDevice(stmtCtx, deviceOperand); @@ -3179,23 +3208,8 @@ genTargetOp(Fortran::lower::AbstractConverter &converter, .getIsTargetDevice()) cp.processNowait(nowaitAttr); - if (outerCombined) { - for (const Fortran::parser::OmpClause &clause : clauseList.v) { - if (const auto &reductionClause = - std::get_if(&clause.u)) { - const auto &objectList{ - std::get(reductionClause->v.t)}; - - for (const Fortran::parser::OmpObject &ompObject : objectList.v) { - if (const auto *name{ - Fortran::parser::Unwrap(ompObject)}) { - if (const Fortran::semantics::Symbol * symbol{name->symbol}) - redSymbols.push_back(symbol); - } - } - } - } - } + if (outerCombined) + cp.processTargetReduction(reductionSymbols); // 5.8.1 Implicit Data-Mapping Attribute Rules // The following code follows the implicit data-mapping rules to map all the @@ -3242,10 +3256,9 @@ genTargetOp(Fortran::lower::AbstractConverter &converter, eleType = refType.getElementType(); // Do a tofrom map for reduction variables. - if (llvm::find(redSymbols, &sym) != redSymbols.end()) { + if (llvm::find(reductionSymbols, &sym) != reductionSymbols.end()) { mapFlag |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM; mapFlag |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO; - captureKind = mlir::omp::VariableCaptureKind::ByRef; } else if (fir::isa_trivial(eleType) || fir::isa_char(eleType)) { captureKind = mlir::omp::VariableCaptureKind::ByCopy; } else if (!fir::isa_builtin_cptr_type(eleType)) {