Skip to content

Commit 52af5b8

Browse files
skatrakskatrak
committed
[MLIR][OpenMP] Improve omp.map.info verification
This patch makes the `map_type` and `map_capture_type` arguments of the `omp.map.info` operation required, which was already an invariant being verified by its users via `verifyMapClause()`. This makes it clearer, as getters no longer return misleading `std::optional` values. Checks for the `mapper_id` argument are moved to a verifier for the operation, rather than being checked by users. Functionally NFC, but not marked as such due to a reordering of arguments in the assembly format of `omp.map.info`.
1 parent ff3341c commit 52af5b8

25 files changed

+127
-127
lines changed

‎flang/docs/OpenMP-descriptor-management.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ omp.target map_entries(%12 -> %arg1, %13 -> %arg2 : !fir.ref<!fir.box<!fir.ptr<!
6868

6969
...
7070
%12 = fir.box_offset %1#1 base_addr : (!fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>) -> !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>
71-
%13 = omp.map.info var_ptr(%1#1 : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.array<?xi32>) var_ptr_ptr(%12 : !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>) map_clauses(tofrom) capture(ByRef) bounds(%11) -> !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>> {name = ""}
71+
%13 = omp.map.info var_ptr(%1#1 : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.array<?xi32>) map_clauses(tofrom) capture(ByRef) var_ptr_ptr(%12 : !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>) bounds(%11) -> !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>> {name = ""}
7272
%14 = omp.map.info var_ptr(%1#1 : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.box<!fir.ptr<!fir.array<?xi32>>>) map_clauses(tofrom) capture(ByRef) members(%13 : !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>) -> !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>> {name = "arg_alloc"}
7373
...
7474
omp.target map_entries(%13 -> %arg1, %14 -> %arg2, %15 -> %arg3 : !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>, !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.ref<i32>) {

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -143,10 +143,10 @@ createMapInfoOp(fir::FirOpBuilder &builder, mlir::Location loc,
143143
varType = mlir::TypeAttr::get(seqType.getEleTy());
144144

145145
mlir::omp::MapInfoOp op = builder.create<mlir::omp::MapInfoOp>(
146-
loc, retTy, baseAddr, varType, varPtrPtr, members, membersIndex, bounds,
146+
loc, retTy, baseAddr, varType,
147147
builder.getIntegerAttr(builder.getIntegerType(64, false), mapType),
148-
mapperId,
149148
builder.getAttr<mlir::omp::VariableCaptureKindAttr>(mapCaptureType),
149+
varPtrPtr, members, membersIndex, bounds, mapperId,
150150
builder.getStringAttr(name), builder.getBoolAttr(partialMap));
151151
return op;
152152
}
@@ -551,7 +551,7 @@ void insertChildMapInfoIntoParent(
551551
// it allows this to work with enter and exit without causing MLIR
552552
// verification issues. The more appropriate thing may be to take
553553
// the "main" map type clause from the directive being used.
554-
uint64_t mapType = indices.second.memberMap[0].getMapType().value_or(0);
554+
uint64_t mapType = indices.second.memberMap[0].getMapType();
555555

556556
llvm::SmallVector<mlir::Value> members;
557557
members.reserve(indices.second.memberMap.size());

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

Lines changed: 18 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -180,13 +180,13 @@ class MapInfoFinalizationPass
180180
// Member of the descriptor pointing at the allocated data
181181
return builder.create<mlir::omp::MapInfoOp>(
182182
loc, baseAddrAddr.getType(), descriptor,
183-
mlir::TypeAttr::get(underlyingVarType), baseAddrAddr,
184-
/*members=*/mlir::SmallVector<mlir::Value>{},
185-
/*membersIndex=*/mlir::ArrayAttr{}, bounds,
183+
mlir::TypeAttr::get(underlyingVarType),
186184
builder.getIntegerAttr(builder.getIntegerType(64, false), mapType),
187-
/*mapperId*/ mlir::FlatSymbolRefAttr(),
188185
builder.getAttr<mlir::omp::VariableCaptureKindAttr>(
189186
mlir::omp::VariableCaptureKind::ByRef),
187+
baseAddrAddr, /*members=*/mlir::SmallVector<mlir::Value>{},
188+
/*membersIndex=*/mlir::ArrayAttr{}, bounds,
189+
/*mapperId*/ mlir::FlatSymbolRefAttr(),
190190
/*name=*/builder.getStringAttr(""),
191191
/*partial_map=*/builder.getBoolAttr(false));
192192
}
@@ -311,8 +311,8 @@ class MapInfoFinalizationPass
311311
assert(mapMemberUsers.size() == 1 &&
312312
"OMPMapInfoFinalization currently only supports single users of a "
313313
"MapInfoOp");
314-
auto baseAddr = genBaseAddrMap(descriptor, op.getBounds(),
315-
op.getMapType().value_or(0), builder);
314+
auto baseAddr =
315+
genBaseAddrMap(descriptor, op.getBounds(), op.getMapType(), builder);
316316
ParentAndPlacement mapUser = mapMemberUsers[0];
317317
adjustMemberIndices(memberIndices, mapUser.index);
318318
llvm::SmallVector<mlir::Value> newMemberOps;
@@ -325,8 +325,8 @@ class MapInfoFinalizationPass
325325
mapUser.parent.setMembersIndexAttr(
326326
builder.create2DI64ArrayAttr(memberIndices));
327327
} else if (!IsHasDeviceAddr) {
328-
auto baseAddr = genBaseAddrMap(descriptor, op.getBounds(),
329-
op.getMapType().value_or(0), builder);
328+
auto baseAddr =
329+
genBaseAddrMap(descriptor, op.getBounds(), op.getMapType(), builder);
330330
newMembers.push_back(baseAddr);
331331
if (!op.getMembers().empty()) {
332332
for (auto &indices : memberIndices)
@@ -346,22 +346,21 @@ class MapInfoFinalizationPass
346346
// one place in the code may differ from that address in another place.
347347
// The contents of the descriptor (the base address in particular) will
348348
// remain unchanged though.
349-
uint64_t MapType = op.getMapType().value_or(0);
349+
uint64_t mapType = op.getMapType();
350350
if (IsHasDeviceAddr) {
351-
MapType |= llvm::to_underlying(
351+
mapType |= llvm::to_underlying(
352352
llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ALWAYS);
353353
}
354354

355355
mlir::omp::MapInfoOp newDescParentMapOp =
356356
builder.create<mlir::omp::MapInfoOp>(
357357
op->getLoc(), op.getResult().getType(), descriptor,
358358
mlir::TypeAttr::get(fir::unwrapRefType(descriptor.getType())),
359-
/*varPtrPtr=*/mlir::Value{}, newMembers, newMembersAttr,
360-
/*bounds=*/mlir::SmallVector<mlir::Value>{},
361359
builder.getIntegerAttr(builder.getIntegerType(64, false),
362-
getDescriptorMapType(MapType, target)),
363-
/*mapperId*/ mlir::FlatSymbolRefAttr(), op.getMapCaptureTypeAttr(),
364-
op.getNameAttr(),
360+
getDescriptorMapType(mapType, target)),
361+
op.getMapCaptureTypeAttr(), /*varPtrPtr=*/mlir::Value{}, newMembers,
362+
newMembersAttr, /*bounds=*/mlir::SmallVector<mlir::Value>{},
363+
/*mapperId*/ mlir::FlatSymbolRefAttr(), op.getNameAttr(),
365364
/*partial_map=*/builder.getBoolAttr(false));
366365
op.replaceAllUsesWith(newDescParentMapOp.getResult());
367366
op->erase();
@@ -656,13 +655,12 @@ class MapInfoFinalizationPass
656655
fieldCoord.getResult(),
657656
mlir::TypeAttr::get(
658657
fir::unwrapRefType(fieldCoord.getResult().getType())),
659-
/*varPtrPtr=*/mlir::Value{},
660-
/*members=*/mlir::ValueRange{},
661-
/*members_index=*/mlir::ArrayAttr{},
662-
/*bounds=*/bounds, op.getMapTypeAttr(),
663-
/*mapperId*/ mlir::FlatSymbolRefAttr(),
658+
op.getMapTypeAttr(),
664659
builder.getAttr<mlir::omp::VariableCaptureKindAttr>(
665660
mlir::omp::VariableCaptureKind::ByRef),
661+
/*varPtrPtr=*/mlir::Value{}, /*members=*/mlir::ValueRange{},
662+
/*members_index=*/mlir::ArrayAttr{}, bounds,
663+
/*mapperId=*/mlir::FlatSymbolRefAttr(),
666664
builder.getStringAttr(op.getNameAttr().strref() + "." +
667665
field + ".implicit_map"),
668666
/*partial_map=*/builder.getBoolAttr(false));

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -85,16 +85,16 @@ class MapsForPrivatizedSymbolsPass
8585
loc, varPtr.getType(), varPtr,
8686
TypeAttr::get(llvm::cast<omp::PointerLikeType>(varPtr.getType())
8787
.getElementType()),
88-
/*varPtrPtr=*/Value{},
89-
/*members=*/SmallVector<Value>{},
90-
/*member_index=*/mlir::ArrayAttr{},
91-
/*bounds=*/ValueRange{},
9288
builder.getIntegerAttr(builder.getIntegerType(64, /*isSigned=*/false),
9389
mapTypeTo),
94-
/*mapperId*/ mlir::FlatSymbolRefAttr(),
9590
builder.getAttr<omp::VariableCaptureKindAttr>(
9691
omp::VariableCaptureKind::ByRef),
97-
StringAttr(), builder.getBoolAttr(false));
92+
/*varPtrPtr=*/Value{},
93+
/*members=*/SmallVector<Value>{},
94+
/*member_index=*/mlir::ArrayAttr{},
95+
/*bounds=*/ValueRange{},
96+
/*mapperId=*/mlir::FlatSymbolRefAttr(), /*name=*/StringAttr(),
97+
builder.getBoolAttr(false));
9898
}
9999
void addMapInfoOp(omp::TargetOp targetOp, omp::MapInfoOp mapInfoOp) {
100100
auto argIface = llvm::cast<omp::BlockArgOpenMPOpInterface>(*targetOp);

‎flang/test/Analysis/AliasAnalysis/alias-analysis-omp-target-1.fir

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ func.func @_QPTestAllocatableArray() {
3030
%8 = arith.subi %7#1, %c1 : index
3131
%9 = omp.map.bounds lower_bound(%c0_1 : index) upper_bound(%8 : index) extent(%7#1 : index) stride(%7#2 : index) start_idx(%6#0 : index) {stride_in_bytes = true}
3232
%10 = fir.box_offset %1#1 base_addr : (!fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>) -> !fir.llvm_ptr<!fir.ref<!fir.array<?xf64>>>
33-
%11 = omp.map.info var_ptr(%1#1 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>, !fir.array<?xf64>) var_ptr_ptr(%10 : !fir.llvm_ptr<!fir.ref<!fir.array<?xf64>>>) map_clauses(implicit, tofrom) capture(ByRef) bounds(%9) -> !fir.llvm_ptr<!fir.ref<!fir.array<?xf64>>> {name = ""}
33+
%11 = omp.map.info var_ptr(%1#1 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>, !fir.array<?xf64>) map_clauses(implicit, tofrom) capture(ByRef) var_ptr_ptr(%10 : !fir.llvm_ptr<!fir.ref<!fir.array<?xf64>>>) bounds(%9) -> !fir.llvm_ptr<!fir.ref<!fir.array<?xf64>>> {name = ""}
3434
%12 = omp.map.info var_ptr(%1#1 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>, !fir.box<!fir.heap<!fir.array<?xf64>>>) map_clauses(implicit, tofrom) capture(ByRef) members(%11 : [0] : !fir.llvm_ptr<!fir.ref<!fir.array<?xf64>>>) -> !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>> {name = "a"}
3535
%13 = fir.load %3#0 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>
3636
%c1_2 = arith.constant 1 : index
@@ -43,7 +43,7 @@ func.func @_QPTestAllocatableArray() {
4343
%17 = arith.subi %16#1, %c1_2 : index
4444
%18 = omp.map.bounds lower_bound(%c0_5 : index) upper_bound(%17 : index) extent(%16#1 : index) stride(%16#2 : index) start_idx(%15#0 : index) {stride_in_bytes = true}
4545
%19 = fir.box_offset %3#1 base_addr : (!fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>) -> !fir.llvm_ptr<!fir.ref<!fir.array<?xf64>>>
46-
%20 = omp.map.info var_ptr(%3#1 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>, !fir.array<?xf64>) var_ptr_ptr(%19 : !fir.llvm_ptr<!fir.ref<!fir.array<?xf64>>>) map_clauses(implicit, tofrom) capture(ByRef) bounds(%18) -> !fir.llvm_ptr<!fir.ref<!fir.array<?xf64>>> {name = ""}
46+
%20 = omp.map.info var_ptr(%3#1 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>, !fir.array<?xf64>) map_clauses(implicit, tofrom) capture(ByRef) var_ptr_ptr(%19 : !fir.llvm_ptr<!fir.ref<!fir.array<?xf64>>>) bounds(%18) -> !fir.llvm_ptr<!fir.ref<!fir.array<?xf64>>> {name = ""}
4747
%21 = omp.map.info var_ptr(%3#1 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>, !fir.box<!fir.heap<!fir.array<?xf64>>>) map_clauses(implicit, tofrom) capture(ByRef) members(%20 : [0] : !fir.llvm_ptr<!fir.ref<!fir.array<?xf64>>>) -> !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>> {name = "b"}
4848
omp.target map_entries(%11 -> %arg0, %12 -> %arg1, %20 -> %arg2, %21 -> %arg3 : !fir.llvm_ptr<!fir.ref<!fir.array<?xf64>>>, !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xf64>>>, !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>) {
4949
%22:2 = hlfir.declare %arg1 {fortran_attrs = #fir.var_attrs<allocatable>, uniq_name = "_QFEa"} : (!fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>) -> (!fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>, !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>)

0 commit comments

Comments
 (0)