Skip to content

Commit 1a2e2ae

Browse files
bhandarkar-pranavronlieb
bhandarkar-pranav
authored and
ronlieb
committed
[Flang][MLIR] - Handle the mapping of subroutine arguments when they are subsequently used inside the region of an omp.target Op (llvm#134967)
This is a fix for llvm#134912 which is a problem with mapping `fir.boxchar<k>` type values to the target i.e an `omp.target` op. There really are two problems. Fixing the first exposed the second. The first problem is that OpenMP lowering of maps in `omp.target` in Flang cannot handle the mapping of a value that doesnt have a defining operation. In other words, a value that is a block argument. This is handled by mapping the value using a `MapInfoOp`. The second problem this fixes is that it adds bounds to `omp.map.info` ops that map `fir.char<k, ?>` types by extracting the length from the corresponding `fir.boxchar`
1 parent ae71285 commit 1a2e2ae

File tree

4 files changed

+133
-40
lines changed

4 files changed

+133
-40
lines changed

‎flang/include/flang/Optimizer/Builder/DirectivesCommon.h

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
#ifndef FORTRAN_OPTIMIZER_BUILDER_DIRECTIVESCOMMON_H_
1818
#define FORTRAN_OPTIMIZER_BUILDER_DIRECTIVESCOMMON_H_
1919

20+
#include "BoxValue.h"
21+
#include "FIRBuilder.h"
2022
#include "flang/Optimizer/Builder/BoxValue.h"
2123
#include "flang/Optimizer/Builder/FIRBuilder.h"
2224
#include "flang/Optimizer/Builder/Todo.h"
@@ -129,6 +131,31 @@ gatherBoundsOrBoundValues(fir::FirOpBuilder &builder, mlir::Location loc,
129131
}
130132
return values;
131133
}
134+
template <typename BoundsOp, typename BoundsType>
135+
mlir::Value
136+
genBoundsOpFromBoxChar(fir::FirOpBuilder &builder, mlir::Location loc,
137+
fir::ExtendedValue dataExv, AddrAndBoundsInfo &info) {
138+
// TODO: Handle info.isPresent.
139+
if (auto boxCharType =
140+
mlir::dyn_cast<fir::BoxCharType>(info.addr.getType())) {
141+
mlir::Type idxTy = builder.getIndexType();
142+
mlir::Type lenType = builder.getCharacterLengthType();
143+
mlir::Type refType = builder.getRefType(boxCharType.getEleTy());
144+
auto unboxed =
145+
builder.create<fir::UnboxCharOp>(loc, refType, lenType, info.addr);
146+
mlir::Value zero = builder.createIntegerConstant(loc, idxTy, 0);
147+
mlir::Value one = builder.createIntegerConstant(loc, idxTy, 1);
148+
mlir::Value extent = unboxed.getResult(1);
149+
mlir::Value stride = one;
150+
mlir::Value ub = builder.create<mlir::arith::SubIOp>(loc, extent, one);
151+
mlir::Type boundTy = builder.getType<mlir::omp::MapBoundsType>();
152+
return builder.create<mlir::omp::MapBoundsOp>(
153+
loc, boundTy, /*lower_bound=*/zero,
154+
/*upper_bound=*/ub, /*extent=*/extent, /*stride=*/stride,
155+
/*stride_in_bytes=*/true, /*start_idx=*/zero);
156+
}
157+
return mlir::Value{};
158+
}
132159

133160
/// Generate the bounds operation from the descriptor information.
134161
template <typename BoundsOp, typename BoundsType>
@@ -248,6 +275,10 @@ genImplicitBoundsOps(fir::FirOpBuilder &builder, AddrAndBoundsInfo &info,
248275
bounds = genBaseBoundsOps<BoundsOp, BoundsType>(builder, loc, dataExv,
249276
dataExvIsAssumedSize);
250277
}
278+
if (characterWithDynamicLen(fir::unwrapRefType(baseOp.getType()))) {
279+
bounds = {genBoundsOpFromBoxChar<BoundsOp, BoundsType>(builder, loc,
280+
dataExv, info)};
281+
}
251282

252283
return bounds;
253284
}

‎flang/lib/Lower/OpenMP/OpenMP.cpp

Lines changed: 21 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -217,59 +217,36 @@ static void bindEntryBlockArgs(lower::AbstractConverter &converter,
217217
assert(args.isValid() && "invalid args");
218218
fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
219219

220-
auto bindSingleMapLike = [&converter,
221-
&firOpBuilder](const semantics::Symbol &sym,
222-
const mlir::BlockArgument &arg) {
223-
// Clones the `bounds` placing them inside the entry block and returns
224-
// them.
225-
auto cloneBound = [&](mlir::Value bound) {
226-
if (mlir::isMemoryEffectFree(bound.getDefiningOp())) {
227-
mlir::Operation *clonedOp = firOpBuilder.clone(*bound.getDefiningOp());
228-
return clonedOp->getResult(0);
229-
}
230-
TODO(converter.getCurrentLocation(),
231-
"target map-like clause operand unsupported bound type");
232-
};
233-
234-
auto cloneBounds = [cloneBound](llvm::ArrayRef<mlir::Value> bounds) {
235-
llvm::SmallVector<mlir::Value> clonedBounds;
236-
llvm::transform(bounds, std::back_inserter(clonedBounds),
237-
[&](mlir::Value bound) { return cloneBound(bound); });
238-
return clonedBounds;
239-
};
240-
220+
auto bindSingleMapLike = [&converter](const semantics::Symbol &sym,
221+
const mlir::BlockArgument &arg) {
241222
fir::ExtendedValue extVal = converter.getSymbolExtendedValue(sym);
242223
auto refType = mlir::dyn_cast<fir::ReferenceType>(arg.getType());
243224
if (refType && fir::isa_builtin_cptr_type(refType.getElementType())) {
244225
converter.bindSymbol(sym, arg);
245226
} else {
246227
extVal.match(
247228
[&](const fir::BoxValue &v) {
248-
converter.bindSymbol(sym,
249-
fir::BoxValue(arg, cloneBounds(v.getLBounds()),
250-
v.getExplicitParameters(),
251-
v.getExplicitExtents()));
229+
converter.bindSymbol(sym, fir::BoxValue(arg, v.getLBounds(),
230+
v.getExplicitParameters(),
231+
v.getExplicitExtents()));
252232
},
253233
[&](const fir::MutableBoxValue &v) {
254234
converter.bindSymbol(
255-
sym, fir::MutableBoxValue(arg, cloneBounds(v.getLBounds()),
235+
sym, fir::MutableBoxValue(arg, v.getLBounds(),
256236
v.getMutableProperties()));
257237
},
258238
[&](const fir::ArrayBoxValue &v) {
259-
converter.bindSymbol(
260-
sym, fir::ArrayBoxValue(arg, cloneBounds(v.getExtents()),
261-
cloneBounds(v.getLBounds()),
262-
v.getSourceBox()));
239+
converter.bindSymbol(sym, fir::ArrayBoxValue(arg, v.getExtents(),
240+
v.getLBounds(),
241+
v.getSourceBox()));
263242
},
264243
[&](const fir::CharArrayBoxValue &v) {
265-
converter.bindSymbol(
266-
sym, fir::CharArrayBoxValue(arg, cloneBound(v.getLen()),
267-
cloneBounds(v.getExtents()),
268-
cloneBounds(v.getLBounds())));
244+
converter.bindSymbol(sym, fir::CharArrayBoxValue(arg, v.getLen(),
245+
v.getExtents(),
246+
v.getLBounds()));
269247
},
270248
[&](const fir::CharBoxValue &v) {
271-
converter.bindSymbol(
272-
sym, fir::CharBoxValue(arg, cloneBound(v.getLen())));
249+
converter.bindSymbol(sym, fir::CharBoxValue(arg, v.getLen()));
273250
},
274251
[&](const fir::UnboxedValue &v) { converter.bindSymbol(sym, arg); },
275252
[&](const auto &) {
@@ -1338,14 +1315,13 @@ static void genBodyOfTargetOp(
13381315
while (!valuesDefinedAbove.empty()) {
13391316
for (mlir::Value val : valuesDefinedAbove) {
13401317
mlir::Operation *valOp = val.getDefiningOp();
1341-
assert(valOp != nullptr);
13421318

13431319
// NOTE: We skip BoxDimsOp's as the lesser of two evils is to map the
13441320
// indices separately, as the alternative is to eventually map the Box,
13451321
// which comes with a fairly large overhead comparatively. We could be
13461322
// more robust about this and check using a BackwardsSlice to see if we
13471323
// run the risk of mapping a box.
1348-
if (mlir::isMemoryEffectFree(valOp) &&
1324+
if (valOp && mlir::isMemoryEffectFree(valOp) &&
13491325
!mlir::isa<fir::BoxDimsOp>(valOp)) {
13501326
mlir::Operation *clonedOp = valOp->clone();
13511327
entryBlock->push_front(clonedOp);
@@ -1358,7 +1334,13 @@ static void genBodyOfTargetOp(
13581334
valOp->replaceUsesWithIf(clonedOp, replace);
13591335
} else {
13601336
auto savedIP = firOpBuilder.getInsertionPoint();
1361-
firOpBuilder.setInsertionPointAfter(valOp);
1337+
1338+
if (valOp)
1339+
firOpBuilder.setInsertionPointAfter(valOp);
1340+
else
1341+
// This means val is a block argument
1342+
firOpBuilder.setInsertionPoint(targetOp);
1343+
13621344
auto copyVal =
13631345
firOpBuilder.createTemporary(val.getLoc(), val.getType());
13641346
firOpBuilder.createStoreWithConvert(copyVal.getLoc(), val, copyVal);

‎flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@
4747
#include <iterator>
4848
#include <numeric>
4949

50+
#define DEBUG_TYPE "omp-map-info-finalization"
51+
#define PDBGS() (llvm::dbgs() << "[" << DEBUG_TYPE << "]: ")
5052
namespace flangomp {
5153
#define GEN_PASS_DEF_MAPINFOFINALIZATIONPASS
5254
#include "flang/Optimizer/OpenMP/Passes.h.inc"
@@ -500,7 +502,38 @@ class MapInfoFinalizationPass
500502
// iterations from previous function scopes.
501503
localBoxAllocas.clear();
502504

503-
// First, walk `omp.map.info` ops to see if any record members should be
505+
// First, walk `omp.map.info` ops to see if any of them have varPtrs
506+
// with an underlying type of fir.char<k, ?>, i.e a character
507+
// with dynamic length. If so, check if they need bounds added.
508+
func->walk([&](mlir::omp::MapInfoOp op) {
509+
if (!op.getBounds().empty())
510+
return;
511+
512+
mlir::Value varPtr = op.getVarPtr();
513+
mlir::Type underlyingVarType = fir::unwrapRefType(varPtr.getType());
514+
515+
if (!fir::characterWithDynamicLen(underlyingVarType))
516+
return;
517+
518+
fir::factory::AddrAndBoundsInfo info =
519+
fir::factory::getDataOperandBaseAddr(
520+
builder, varPtr, /*isOptional=*/false, varPtr.getLoc());
521+
fir::ExtendedValue extendedValue =
522+
hlfir::translateToExtendedValue(varPtr.getLoc(), builder,
523+
hlfir::Entity{info.addr},
524+
/*continguousHint=*/true)
525+
.first;
526+
builder.setInsertionPoint(op);
527+
llvm::SmallVector<mlir::Value> boundsOps =
528+
fir::factory::genImplicitBoundsOps<mlir::omp::MapBoundsOp,
529+
mlir::omp::MapBoundsType>(
530+
builder, info, extendedValue,
531+
/*dataExvIsAssumedSize=*/false, varPtr.getLoc());
532+
533+
op.getBoundsMutable().append(boundsOps);
534+
});
535+
536+
// Next, walk `omp.map.info` ops to see if any record members should be
504537
// implicitly mapped.
505538
// TODO/FIXME/UPDATE: I believe we need to add implicit capture of
506539
// allocatable members of arbitrary depths for this before we can

‎flang/test/Lower/OpenMP/map-character.f90

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
! RUN: %flang_fc1 -emit-hlfir -fopenmp -o - %s 2>&1 | FileCheck %s
2+
3+
subroutine TestOfCharacter(a0, a1, l)
4+
character(len=*), intent(in) :: a0
5+
character(len=*), intent(inout):: a1
6+
integer, intent(in) :: l
7+
8+
!$omp target map(to:a0) map(from: a1)
9+
a1 = a0
10+
!$omp end target
11+
end subroutine TestOfCharacter
12+
13+
14+
!CHECK: func.func @_QPtestofcharacter(%[[ARG0:.*]]: !fir.boxchar<1> {{.*}}, %[[ARG1:.*]]: !fir.boxchar<1> {{.*}}
15+
!CHECK: %[[A0_BOXCHAR_ALLOCA:.*]] = fir.alloca !fir.boxchar<1>
16+
!CHECK: %[[A1_BOXCHAR_ALLOCA:.*]] = fir.alloca !fir.boxchar<1>
17+
!CHECK: %[[UNBOXED_ARG0:.*]]:2 = fir.unboxchar %[[ARG0]] : (!fir.boxchar<1>) -> (!fir.ref<!fir.char<1,?>>, index)
18+
!CHECK: %[[A0_DECL:.*]]:2 = hlfir.declare %[[UNBOXED_ARG0]]#0 typeparams %[[UNBOXED_ARG0]]#1 dummy_scope {{.*}} -> (!fir.boxchar<1>, !fir.ref<!fir.char<1,?>>)
19+
!CHECK: %[[UNBOXED_ARG1:.*]]:2 = fir.unboxchar %[[ARG1]] : (!fir.boxchar<1>) -> (!fir.ref<!fir.char<1,?>>, index)
20+
!CHECK: %[[A1_DECL:.*]]:2 = hlfir.declare %[[UNBOXED_ARG1]]#0 typeparams %[[UNBOXED_ARG1]]#1 dummy_scope {{.*}} -> (!fir.boxchar<1>, !fir.ref<!fir.char<1,?>>)
21+
!CHECK: %[[UNBOXED_A0_DECL:.*]]:2 = fir.unboxchar %[[A0_DECL]]#0 : (!fir.boxchar<1>) -> (!fir.ref<!fir.char<1,?>>, index)
22+
!CHECK: %[[A0_LB:.*]] = arith.constant 0 : index
23+
!CHECK: %[[A0_STRIDE:.*]] = arith.constant 1 : index
24+
!CHECK: %[[A0_UB:.*]] = arith.subi %[[UNBOXED_A0_DECL]]#1, %[[A0_STRIDE]] : index
25+
!CHECK: %[[A0_BOUNDS:.*]] = omp.map.bounds lower_bound(%[[A0_LB]] : index) upper_bound(%[[A0_UB]] : index) extent(%[[UNBOXED_A0_DECL]]#1 : index)
26+
!CHECK-SAME: stride(%[[A0_STRIDE]] : index) start_idx(%[[A0_LB]] : index) {stride_in_bytes = true}
27+
!CHECK: %[[A0_MAP:.*]] = omp.map.info var_ptr(%[[A0_DECL]]#1 : !fir.ref<!fir.char<1,?>>, !fir.char<1,?>) map_clauses(to) capture(ByRef) bounds(%[[A0_BOUNDS]]) -> !fir.ref<!fir.char<1,?>> {name = "a0"}
28+
!CHECK: %[[UNBOXED_A1_DECL:.*]]:2 = fir.unboxchar %[[A1_DECL]]#0 : (!fir.boxchar<1>) -> (!fir.ref<!fir.char<1,?>>, index)
29+
!CHECK: %[[A1_LB:.*]] = arith.constant 0 : index
30+
!CHECK: %[[A1_STRIDE:.*]] = arith.constant 1 : index
31+
!CHECK: %[[A1_UB:.*]] = arith.subi %[[UNBOXED_A1_DECL]]#1, %[[A1_STRIDE]] : index
32+
!CHECK: %[[A1_BOUNDS:.*]] = omp.map.bounds lower_bound(%[[A1_LB]] : index) upper_bound(%[[A1_UB]] : index) extent(%[[UNBOXED_A1_DECL]]#1 : index)
33+
!CHECKL-SAME: stride(%[[A1_STRIDE]] : index) start_idx(%[[A1_LB]] : index) {stride_in_bytes = true}
34+
!CHECK: %[[A1_MAP:.*]] = omp.map.info var_ptr(%[[A1_DECL]]#1 : !fir.ref<!fir.char<1,?>>, !fir.char<1,?>) map_clauses(from) capture(ByRef) bounds(%[[A1_BOUNDS]]) -> !fir.ref<!fir.char<1,?>> {name = "a1"}
35+
!CHECK: fir.store %[[ARG1]] to %[[A1_BOXCHAR_ALLOCA]] : !fir.ref<!fir.boxchar<1>>
36+
!CHECK: %[[A1_BOXCHAR_MAP:.*]] = omp.map.info var_ptr(%[[A1_BOXCHAR_ALLOCA]] : !fir.ref<!fir.boxchar<1>>, !fir.boxchar<1>) map_clauses(implicit, to) capture(ByRef) -> !fir.ref<!fir.boxchar<1>> {name = ""}
37+
!CHECK: fir.store %[[ARG0]] to %[[A0_BOXCHAR_ALLOCA]] : !fir.ref<!fir.boxchar<1>>
38+
!CHECK: %[[A0_BOXCHAR_MAP:.*]] = omp.map.info var_ptr(%[[A0_BOXCHAR_ALLOCA]] : !fir.ref<!fir.boxchar<1>>, !fir.boxchar<1>) map_clauses(implicit, to) capture(ByRef) -> !fir.ref<!fir.boxchar<1>> {name = ""}
39+
40+
!CHECK: omp.target map_entries(%[[A0_MAP]] -> %[[TGT_A0:.*]], %[[A1_MAP]] -> %[[TGT_A1:.*]], %[[A1_BOXCHAR_MAP]] -> %[[TGT_A1_BOXCHAR:.*]], %[[A0_BOXCHAR_MAP]] -> %[[TGT_A0_BOXCHAR:.*]] : !fir.ref<!fir.char<1,?>>, !fir.ref<!fir.char<1,?>>, !fir.ref<!fir.boxchar<1>>, !fir.ref<!fir.boxchar<1>>) {
41+
!CHECK: %[[TGT_A0_BC_LD:.*]] = fir.load %[[TGT_A0_BOXCHAR]] : !fir.ref<!fir.boxchar<1>>
42+
!CHECK: %[[TGT_A1_BC_LD:.*]] = fir.load %[[TGT_A1_BOXCHAR]] : !fir.ref<!fir.boxchar<1>>
43+
!CHECK: %[[UNBOXED_TGT_A1:.*]]:2 = fir.unboxchar %[[TGT_A1_BC_LD]] : (!fir.boxchar<1>) -> (!fir.ref<!fir.char<1,?>>, index)
44+
!CHECK: %[[UNBOXED_TGT_A0:.*]]:2 = fir.unboxchar %[[TGT_A0_BC_LD]] : (!fir.boxchar<1>) -> (!fir.ref<!fir.char<1,?>>, index)
45+
!CHECK: %[[TGT_A0_DECL:.*]]:2 = hlfir.declare %[[TGT_A0]] typeparams %[[UNBOXED_TGT_A0]]#1 {{.*}} -> (!fir.boxchar<1>, !fir.ref<!fir.char<1,?>>)
46+
!CHECK: %[[TGT_A1_DECL:.*]]:2 = hlfir.declare %[[TGT_A1]] typeparams %[[UNBOXED_TGT_A1]]#1 {{.*}} -> (!fir.boxchar<1>, !fir.ref<!fir.char<1,?>>)
47+

0 commit comments

Comments
 (0)