Skip to content

Commit a0406ce

Browse files
authored
[flang][OpenMP] Add hostIsSource paramemter to copyHostAssociateVar (#123162)
This fixes a bug when the same variable is used in `firstprivate` and `lastprivate` clauses on the same construct. The issue boils down to the fact that `copyHostAssociateVar` was deciding the direction of the copy assignment (i.e. the `lhs` and `rhs`) based on whether the `copyAssignIP` parameter is set. This is not the best way to do it since it is not related to whether we doing a copy from host to localized copy or the other way around. When we set the insertion for `firstprivate` in delayed privatization, this resulted in switching the direction of the copy assignment. Instead, this PR adds a new paramter to explicitely tell the function the direction of the assignment. This is a follow up PR for #122471, only the latest commit is relevant.
1 parent 6b3ba66 commit a0406ce

File tree

5 files changed

+59
-17
lines changed

5 files changed

+59
-17
lines changed

flang/include/flang/Lower/AbstractConverter.h

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -130,9 +130,18 @@ class AbstractConverter {
130130
virtual void
131131
createHostAssociateVarCloneDealloc(const Fortran::semantics::Symbol &sym) = 0;
132132

133-
virtual void copyHostAssociateVar(
134-
const Fortran::semantics::Symbol &sym,
135-
mlir::OpBuilder::InsertPoint *copyAssignIP = nullptr) = 0;
133+
/// For a host-associated symbol (a symbol associated with another symbol from
134+
/// an enclosing scope), either:
135+
///
136+
/// * if \p hostIsSource == true: copy \p sym's value *from* its corresponding
137+
/// host symbol,
138+
///
139+
/// * if \p hostIsSource == false: copy \p sym's value *to* its corresponding
140+
/// host symbol.
141+
virtual void
142+
copyHostAssociateVar(const Fortran::semantics::Symbol &sym,
143+
mlir::OpBuilder::InsertPoint *copyAssignIP = nullptr,
144+
bool hostIsSource = true) = 0;
136145

137146
virtual void copyVar(mlir::Location loc, mlir::Value dst, mlir::Value src,
138147
fir::FortranVariableFlagsEnum attrs) = 0;

flang/lib/Lower/Bridge.cpp

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -891,9 +891,10 @@ class FirConverter : public Fortran::lower::AbstractConverter {
891891
isPointer, Fortran::semantics::Symbol::Flags());
892892
}
893893

894-
void copyHostAssociateVar(
895-
const Fortran::semantics::Symbol &sym,
896-
mlir::OpBuilder::InsertPoint *copyAssignIP = nullptr) override final {
894+
void
895+
copyHostAssociateVar(const Fortran::semantics::Symbol &sym,
896+
mlir::OpBuilder::InsertPoint *copyAssignIP = nullptr,
897+
bool hostIsSource = true) override final {
897898
// 1) Fetch the original copy of the variable.
898899
assert(sym.has<Fortran::semantics::HostAssocDetails>() &&
899900
"No host-association found");
@@ -908,16 +909,14 @@ class FirConverter : public Fortran::lower::AbstractConverter {
908909
"Host and associated symbol boxes are the same");
909910

910911
// 3) Perform the assignment.
911-
mlir::OpBuilder::InsertPoint insPt = builder->saveInsertionPoint();
912+
mlir::OpBuilder::InsertionGuard guard(*builder);
912913
if (copyAssignIP && copyAssignIP->isSet())
913914
builder->restoreInsertionPoint(*copyAssignIP);
914915
else
915916
builder->setInsertionPointAfter(sb.getAddr().getDefiningOp());
916917

917918
Fortran::lower::SymbolBox *lhs_sb, *rhs_sb;
918-
if (copyAssignIP && copyAssignIP->isSet() &&
919-
sym.test(Fortran::semantics::Symbol::Flag::OmpLastPrivate)) {
920-
// lastprivate case
919+
if (!hostIsSource) {
921920
lhs_sb = &hsb;
922921
rhs_sb = &sb;
923922
} else {
@@ -926,11 +925,6 @@ class FirConverter : public Fortran::lower::AbstractConverter {
926925
}
927926

928927
copyVar(sym, *lhs_sb, *rhs_sb, sym.flags());
929-
930-
if (copyAssignIP && copyAssignIP->isSet() &&
931-
sym.test(Fortran::semantics::Symbol::Flag::OmpLastPrivate)) {
932-
builder->restoreInsertionPoint(insPt);
933-
}
934928
}
935929

936930
void genEval(Fortran::lower::pft::Evaluation &eval,

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ void DataSharingProcessor::copyFirstPrivateSymbol(
145145
void DataSharingProcessor::copyLastPrivateSymbol(
146146
const semantics::Symbol *sym, mlir::OpBuilder::InsertPoint *lastPrivIP) {
147147
if (sym->test(semantics::Symbol::Flag::OmpLastPrivate))
148-
converter.copyHostAssociateVar(*sym, lastPrivIP);
148+
converter.copyHostAssociateVar(*sym, lastPrivIP, /*hostIsSource=*/false);
149149
}
150150

151151
void DataSharingProcessor::collectOmpObjectListSymbol(

flang/lib/Lower/OpenMP/OpenMP.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2082,7 +2082,7 @@ genSectionsOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
20822082
const auto &objList = std::get<ObjectList>(lastp->t);
20832083
for (const Object &object : objList) {
20842084
semantics::Symbol *sym = object.sym();
2085-
converter.copyHostAssociateVar(*sym, &insp);
2085+
converter.copyHostAssociateVar(*sym, &insp, /*hostIsSource=*/false);
20862086
}
20872087
}
20882088
}
Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
! RUN: %flang_fc1 -fopenmp -emit-hlfir %s -o - | FileCheck %s
2+
3+
subroutine first_and_lastprivate
4+
integer i
5+
integer :: var = 1
6+
7+
!$omp parallel do firstprivate(var) lastprivate(var)
8+
do i=1,1
9+
end do
10+
!$omp end parallel do
11+
end subroutine
12+
13+
! CHECK: omp.private {type = firstprivate} @{{.*}}Evar_firstprivate_ref_i32 : {{.*}} alloc {
14+
! CHECK: %[[ALLOC:.*]] = fir.alloca i32 {{.*}}
15+
! CHECK: %[[ALLOC_DECL:.*]]:2 = hlfir.declare %[[ALLOC]]
16+
! CHECK: omp.yield(%[[ALLOC_DECL]]#0 : !fir.ref<i32>)
17+
! CHECK: } copy {
18+
! CHECK: ^{{.*}}(%[[ORIG_REF:.*]]: {{.*}}, %[[PRIV_REF:.*]]: {{.*}}):
19+
! CHECK: %[[ORIG_VAL:.*]] = fir.load %[[ORIG_REF]]
20+
! CHECK: hlfir.assign %[[ORIG_VAL]] to %[[PRIV_REF]]
21+
! CHECK: omp.yield(%[[PRIV_REF]] : !fir.ref<i32>)
22+
! CHECK: }
23+
24+
! CHECK: func.func @{{.*}}first_and_lastprivate()
25+
! CHECK: %[[ORIG_VAR_DECL:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "{{.*}}Evar"}
26+
! CHECK: omp.parallel {
27+
! CHECK: omp.barrier
28+
! CHECK: omp.wsloop private(@{{.*}}var_firstprivate_ref_i32 {{.*}}) {
29+
! CHECK: omp.loop_nest {{.*}} {
30+
! CHECK: %[[PRIV_VAR_DECL:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "{{.*}}Evar"}
31+
! CHECK: fir.if %{{.*}} {
32+
! CHECK: %[[PRIV_VAR_VAL:.*]] = fir.load %[[PRIV_VAR_DECL]]#0 : !fir.ref<i32>
33+
! CHECK: hlfir.assign %[[PRIV_VAR_VAL]] to %[[ORIG_VAR_DECL]]#0
34+
! CHECK: }
35+
! CHECK: omp.yield
36+
! CHECK: }
37+
! CHECK: }
38+
! CHECK: omp.terminator
39+
! CHECK: }

0 commit comments

Comments
 (0)