[Flang][OpenMP] Fix mapping of character type with LEN > 1 specified #154172
Conversation
agozillon
requested review from
ergawy,
skatrak,
jsjodin,
abidh,
mjklemm,
DominikAdamski,
dpalermo,
TIFitis,
kparzysz and
bhandarkar-pranav
llvmbot
added
flang
|
@llvm/pr-subscribers-offload @llvm/pr-subscribers-flang-fir-hlfir Author: None (agozillon) ChangesCurrently, there's a number of issues with mapping characters with LEN's specified (strings effectively). They're represented as a char type in FIR with a len parameter, and then later on they're expanded into an array of characters when we're translating to the LLVM dialect. However, we don't generate a bounds for these at lowering. The fix in this PR for this is to generate a bounds from the LEN parameter and attatch it to the map on lowering from FIR to the LLVM dialect when we encounter this type. Full diff: https://github.com/llvm/llvm-project/pull/154172.diff 2 Files Affected:
diff --git a/flang/lib/Optimizer/CodeGen/CodeGenOpenMP.cpp b/flang/lib/Optimizer/CodeGen/CodeGenOpenMP.cpp
index 97912bda79b08..1c3fc04779377 100644
--- a/flang/lib/Optimizer/CodeGen/CodeGenOpenMP.cpp
+++ b/flang/lib/Optimizer/CodeGen/CodeGenOpenMP.cpp
@@ -60,6 +60,21 @@ struct MapInfoOpConversion
: public OpenMPFIROpConversion<mlir::omp::MapInfoOp> {
using OpenMPFIROpConversion::OpenMPFIROpConversion;
+ mlir::omp::MapBoundsOp
+ createBoundsForCharString(mlir::ConversionPatternRewriter &rewriter,
+ unsigned int len, mlir::Location loc) const {
+ mlir::Type i64Ty = rewriter.getIntegerType(64);
+ auto lBound = mlir::LLVM::ConstantOp::create(rewriter, loc, i64Ty, 0);
+ auto uBoundAndExt =
+ mlir::LLVM::ConstantOp::create(rewriter, loc, i64Ty, len - 1);
+ auto stride = mlir::LLVM::ConstantOp::create(rewriter, loc, i64Ty, 1);
+ auto baseLb = mlir::LLVM::ConstantOp::create(rewriter, loc, i64Ty, 1);
+ auto mapBoundType = rewriter.getType<mlir::omp::MapBoundsType>();
+ return mlir::omp::MapBoundsOp::create(rewriter, loc, mapBoundType, lBound,
+ uBoundAndExt, uBoundAndExt, stride,
+ /*strideInBytes*/ false, baseLb);
+ }
+
llvm::LogicalResult
matchAndRewrite(mlir::omp::MapInfoOp curOp, OpAdaptor adaptor,
mlir::ConversionPatternRewriter &rewriter) const override {
@@ -69,13 +84,58 @@ struct MapInfoOpConversion
return mlir::failure();
llvm::SmallVector<mlir::NamedAttribute> newAttrs;
- mlir::omp::MapInfoOp newOp;
+ mlir::omp::MapBoundsOp mapBoundsOp;
for (mlir::NamedAttribute attr : curOp->getAttrs()) {
if (auto typeAttr = mlir::dyn_cast<mlir::TypeAttr>(attr.getValue())) {
mlir::Type newAttr;
if (fir::isTypeWithDescriptor(typeAttr.getValue())) {
newAttr = lowerTy().convertBoxTypeAsStruct(
mlir::cast<fir::BaseBoxType>(typeAttr.getValue()));
+ } else if (fir::isa_char_string(fir::unwrapSequenceType(
+ fir::unwrapPassByRefType(typeAttr.getValue()))) &&
+ !characterWithDynamicLen(
+ fir::unwrapPassByRefType(typeAttr.getValue()))) {
+ // Characters with a LEN param are represented as char
+ // arrays/strings, the initial lowering doesn't generate
+ // bounds for these, however, we require them to map the
+ // data appropriately in the later lowering stages. This
+ // is to prevent the need for unecessary caveats
+ // specific to Flang. We also strip the array from the
+ // type so that all variations of strings are treated
+ // identically and there's no caveats or specialisations
+ // required in the later stages. As an example, Boxed
+ // char strings will emit a single char array no matter
+ // the number of dimensions caused by additional array
+ // dimensions which needs specialised for, as it differs
+ // from the non-box variation which will emit each array
+ // wrapping the character array, e.g. given a type of
+ // the same dimensions, if one is boxed, the types would
+ // end up:
+ //
+ // array<i8 x 16>
+ // vs
+ // array<10 x array< 10 x array<i8 x 16>>>
+ //
+ // This means we have to treat one specially in the
+ // lowering. So we try to "canonicalize" it here.
+ // TODO: Handle dynamic LEN characters.
+ if (auto ct = mlir::dyn_cast_or_null<fir::CharacterType>(
+ fir::unwrapSequenceType(typeAttr.getValue()))) {
+ newAttr = converter->convertType(
+ fir::unwrapSequenceType(typeAttr.getValue()));
+ if (auto type = mlir::dyn_cast<mlir::LLVM::LLVMArrayType>(newAttr))
+ newAttr = type.getElementType();
+ // We do not generate for device, as MapBoundsOps are
+ // unsupported, as they're currently unused.
+ auto offloadMod =
+ llvm::dyn_cast_or_null<mlir::omp::OffloadModuleInterface>(
+ *curOp->getParentOfType<mlir::ModuleOp>());
+ if (!offloadMod.getIsTargetDevice())
+ mapBoundsOp = createBoundsForCharString(rewriter, ct.getLen(),
+ curOp.getLoc());
+ } else {
+ newAttr = converter->convertType(typeAttr.getValue());
+ }
} else {
newAttr = converter->convertType(typeAttr.getValue());
}
@@ -85,8 +145,13 @@ struct MapInfoOpConversion
}
}
- rewriter.replaceOpWithNewOp<mlir::omp::MapInfoOp>(
+ auto newOp = rewriter.replaceOpWithNewOp<mlir::omp::MapInfoOp>(
curOp, resTypes, adaptor.getOperands(), newAttrs);
+ if (mapBoundsOp) {
+ rewriter.startOpModification(newOp);
+ newOp.getBoundsMutable().append(mlir::ValueRange{mapBoundsOp});
+ rewriter.finalizeOpModification(newOp);
+ }
return mlir::success();
}
diff --git a/flang/test/Fir/OpenMP/bounds-generation-for-char-arrays.f90 b/flang/test/Fir/OpenMP/bounds-generation-for-char-arrays.f90
new file mode 100644
index 0000000000000..d9d54ee72edb8
--- /dev/null
+++ b/flang/test/Fir/OpenMP/bounds-generation-for-char-arrays.f90
@@ -0,0 +1,96 @@
+// RUN: fir-opt --cfg-conversion --fir-to-llvm-ir="target=aarch64-unknown-linux-gnu" %s | FileCheck %s
+
+module attributes {omp.is_target_device = false} {
+ func.func @_QPchar_array(%arg0 : !fir.ref<!fir.array<10x10x!fir.char<1,16>>>) {
+ %c9 = arith.constant 9 : index
+ %c0 = arith.constant 0 : index
+ %c1 = arith.constant 1 : index
+ %c10 = arith.constant 10 : index
+ %0 = omp.map.bounds lower_bound(%c0 : index) upper_bound(%c9 : index) extent(%c10 : index) stride(%c1 : index) start_idx(%c1 : index)
+ %1 = omp.map.bounds lower_bound(%c0 : index) upper_bound(%c9 : index) extent(%c10 : index) stride(%c1 : index) start_idx(%c1 : index)
+ %2 = omp.map.info var_ptr(%arg0 : !fir.ref<!fir.array<10x10x!fir.char<1,16>>>, !fir.array<10x10x!fir.char<1,16>>) map_clauses(tofrom) capture(ByRef) bounds(%0, %1) -> !fir.ref<!fir.array<10x10x!fir.char<1,16>>> {name = ""}
+ omp.target map_entries(%2 -> %arg1 : !fir.ref<!fir.array<10x10x!fir.char<1,16>>>) {
+ omp.terminator
+ }
+ return
+ }
+
+// CHECK-LABEL: llvm.func @_QPchar_array(
+// CHECK-SAME: %[[ARG0:.*]]: !llvm.ptr) {
+// CHECK: %[[VAL_0:.*]] = llvm.mlir.constant(9 : index) : i64
+// CHECK: %[[VAL_1:.*]] = llvm.mlir.constant(0 : index) : i64
+// CHECK: %[[VAL_2:.*]] = llvm.mlir.constant(1 : index) : i64
+// CHECK: %[[VAL_3:.*]] = llvm.mlir.constant(10 : index) : i64
+// CHECK: %[[VAL_4:.*]] = omp.map.bounds lower_bound(%[[VAL_1]] : i64) upper_bound(%[[VAL_0]] : i64) extent(%[[VAL_3]] : i64) stride(%[[VAL_2]] : i64) start_idx(%[[VAL_2]] : i64)
+// CHECK: %[[VAL_5:.*]] = omp.map.bounds lower_bound(%[[VAL_1]] : i64) upper_bound(%[[VAL_0]] : i64) extent(%[[VAL_3]] : i64) stride(%[[VAL_2]] : i64) start_idx(%[[VAL_2]] : i64)
+// CHECK: %[[VAL_6:.*]] = llvm.mlir.constant(0 : i64) : i64
+// CHECK: %[[VAL_7:.*]] = llvm.mlir.constant(15 : i64) : i64
+// CHECK: %[[VAL_8:.*]] = llvm.mlir.constant(1 : i64) : i64
+// CHECK: %[[VAL_9:.*]] = llvm.mlir.constant(1 : i64) : i64
+// CHECK: %[[VAL_10:.*]] = omp.map.bounds lower_bound(%[[VAL_6]] : i64) upper_bound(%[[VAL_7]] : i64) extent(%[[VAL_7]] : i64) stride(%[[VAL_8]] : i64) start_idx(%[[VAL_9]] : i64)
+// CHECK: %[[VAL_11:.*]] = omp.map.info var_ptr(%[[ARG0]] : !llvm.ptr, i8) map_clauses(tofrom) capture(ByRef) bounds(%[[VAL_4]], %[[VAL_5]], %[[VAL_10]]) -> !llvm.ptr {name = ""}
+// CHECK: omp.target map_entries(%[[VAL_11]] -> %[[VAL_12:.*]] : !llvm.ptr) {
+// CHECK: omp.terminator
+// CHECK: }
+// CHECK: llvm.return
+// CHECK: }
+
+ func.func @_QPallocatable_char_array(%arg0 : !fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>>) {
+ %c1 = arith.constant 1 : index
+ %c0 = arith.constant 0 : index
+ %0 = fir.load %arg0 : !fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>>
+ %1:3 = fir.box_dims %0, %c0 : (!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>, index) -> (index, index, index)
+ %2 = arith.subi %1#1, %c1 : index
+ %3 = omp.map.bounds lower_bound(%c0 : index) upper_bound(%2 : index) extent(%1#1 : index) stride(%1#2 : index) start_idx(%1#0 : index) {stride_in_bytes = true}
+ %4 = arith.muli %1#2, %1#1 : index
+ %5:3 = fir.box_dims %0, %c1 : (!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>, index) -> (index, index, index)
+ %6 = arith.subi %5#1, %c1 : index
+ %7 = omp.map.bounds lower_bound(%c0 : index) upper_bound(%6 : index) extent(%5#1 : index) stride(%4 : index) start_idx(%5#0 : index) {stride_in_bytes = true}
+ %8 = fir.box_offset %arg0 base_addr : (!fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>>) -> !fir.llvm_ptr<!fir.ref<!fir.array<?x?x!fir.char<1,16>>>>
+ %9 = omp.map.info var_ptr(%arg0 : !fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>>, !fir.char<1,16>) map_clauses(tofrom) capture(ByRef) var_ptr_ptr(%8 : !fir.llvm_ptr<!fir.ref<!fir.array<?x?x!fir.char<1,16>>>>) bounds(%3, %7) -> !fir.llvm_ptr<!fir.ref<!fir.array<?x?x!fir.char<1,16>>>> {name = ""}
+ %10 = omp.map.info var_ptr(%arg0 : !fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>>, !fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>) map_clauses(to) capture(ByRef) members(%9 : [0] : !fir.llvm_ptr<!fir.ref<!fir.array<?x?x!fir.char<1,16>>>>) -> !fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>> {name = "csv_chem_list_a"}
+ omp.target map_entries(%10 -> %arg1, %9 -> %arg2 : !fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>>, !fir.llvm_ptr<!fir.ref<!fir.array<?x?x!fir.char<1,16>>>>) {
+ omp.terminator
+ }
+ return
+ }
+
+// CHECK-LABEL: llvm.func @_QPallocatable_char_array(
+// CHECK-SAME: %[[ARG0:.*]]: !llvm.ptr) {
+// CHECK: %[[VAL_0:.*]] = llvm.mlir.constant(1 : i32) : i32
+// CHECK: %[[VAL_1:.*]] = llvm.alloca %[[VAL_0]] x !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<2 x array<3 x i64>>)> {alignment = 8 : i64} : (i32) -> !llvm.ptr
+// CHECK: %[[VAL_2:.*]] = llvm.mlir.constant(1 : index) : i64
+// CHECK: %[[VAL_3:.*]] = llvm.mlir.constant(0 : index) : i64
+// CHECK: %[[VAL_4:.*]] = llvm.mlir.constant(72 : i32) : i32
+// CHECK: "llvm.intr.memcpy"(%[[VAL_1]], %[[ARG0]], %[[VAL_4]]) <{isVolatile = false}> : (!llvm.ptr, !llvm.ptr, i32) -> ()
+// CHECK: %[[VAL_5:.*]] = llvm.getelementptr %[[VAL_1]][0, 7, %[[VAL_3]], 0] : (!llvm.ptr, i64) -> !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<2 x array<3 x i64>>)>
+// CHECK: %[[VAL_6:.*]] = llvm.load %[[VAL_5]] : !llvm.ptr -> i64
+// CHECK: %[[VAL_7:.*]] = llvm.getelementptr %[[VAL_1]][0, 7, %[[VAL_3]], 1] : (!llvm.ptr, i64) -> !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<2 x array<3 x i64>>)>
+// CHECK: %[[VAL_8:.*]] = llvm.load %[[VAL_7]] : !llvm.ptr -> i64
+// CHECK: %[[VAL_9:.*]] = llvm.getelementptr %[[VAL_1]][0, 7, %[[VAL_3]], 2] : (!llvm.ptr, i64) -> !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<2 x array<3 x i64>>)>
+// CHECK: %[[VAL_10:.*]] = llvm.load %[[VAL_9]] : !llvm.ptr -> i64
+// CHECK: %[[VAL_11:.*]] = llvm.sub %[[VAL_8]], %[[VAL_2]] : i64
+// CHECK: %[[VAL_12:.*]] = omp.map.bounds lower_bound(%[[VAL_3]] : i64) upper_bound(%[[VAL_11]] : i64) extent(%[[VAL_8]] : i64) stride(%[[VAL_10]] : i64) start_idx(%[[VAL_6]] : i64) {stride_in_bytes = true}
+// CHECK: %[[VAL_13:.*]] = llvm.mul %[[VAL_10]], %[[VAL_8]] : i64
+// CHECK: %[[VAL_14:.*]] = llvm.getelementptr %[[VAL_1]][0, 7, %[[VAL_2]], 0] : (!llvm.ptr, i64) -> !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<2 x array<3 x i64>>)>
+// CHECK: %[[VAL_15:.*]] = llvm.load %[[VAL_14]] : !llvm.ptr -> i64
+// CHECK: %[[VAL_16:.*]] = llvm.getelementptr %[[VAL_1]][0, 7, %[[VAL_2]], 1] : (!llvm.ptr, i64) -> !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<2 x array<3 x i64>>)>
+// CHECK: %[[VAL_17:.*]] = llvm.load %[[VAL_16]] : !llvm.ptr -> i64
+// CHECK: %[[VAL_18:.*]] = llvm.getelementptr %[[VAL_1]][0, 7, %[[VAL_2]], 2] : (!llvm.ptr, i64) -> !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<2 x array<3 x i64>>)>
+// CHECK: %[[VAL_19:.*]] = llvm.load %[[VAL_18]] : !llvm.ptr -> i64
+// CHECK: %[[VAL_20:.*]] = llvm.sub %[[VAL_17]], %[[VAL_2]] : i64
+// CHECK: %[[VAL_21:.*]] = omp.map.bounds lower_bound(%[[VAL_3]] : i64) upper_bound(%[[VAL_20]] : i64) extent(%[[VAL_17]] : i64) stride(%[[VAL_13]] : i64) start_idx(%[[VAL_15]] : i64) {stride_in_bytes = true}
+// CHECK: %[[VAL_22:.*]] = llvm.getelementptr %[[ARG0]][0, 0] : (!llvm.ptr) -> !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<2 x array<3 x i64>>)>
+// CHECK: %[[VAL_23:.*]] = llvm.mlir.constant(0 : i64) : i64
+// CHECK: %[[VAL_24:.*]] = llvm.mlir.constant(15 : i64) : i64
+// CHECK: %[[VAL_25:.*]] = llvm.mlir.constant(1 : i64) : i64
+// CHECK: %[[VAL_26:.*]] = llvm.mlir.constant(1 : i64) : i64
+// CHECK: %[[VAL_27:.*]] = omp.map.bounds lower_bound(%[[VAL_23]] : i64) upper_bound(%[[VAL_24]] : i64) extent(%[[VAL_24]] : i64) stride(%[[VAL_25]] : i64) start_idx(%[[VAL_26]] : i64)
+// CHECK: %[[VAL_28:.*]] = omp.map.info var_ptr(%[[ARG0]] : !llvm.ptr, i8) map_clauses(tofrom) capture(ByRef) var_ptr_ptr(%[[VAL_22]] : !llvm.ptr) bounds(%[[VAL_12]], %[[VAL_21]], %[[VAL_27]]) -> !llvm.ptr {name = ""}
+// CHECK: %[[VAL_29:.*]] = omp.map.info var_ptr(%[[ARG0]] : !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<2 x array<3 x i64>>)>) map_clauses(to) capture(ByRef) members(%[[VAL_28]] : [0] : !llvm.ptr) -> !llvm.ptr {name = "csv_chem_list_a"}
+// CHECK: omp.target map_entries(%[[VAL_29]] -> %[[VAL_30:.*]], %[[VAL_28]] -> %[[VAL_31:.*]] : !llvm.ptr, !llvm.ptr) {
+// CHECK: omp.terminator
+// CHECK: }
+// CHECK: llvm.return
+// CHECK: }
+}
|
|
@llvm/pr-subscribers-flang-codegen Author: None (agozillon) ChangesCurrently, there's a number of issues with mapping characters with LEN's specified (strings effectively). They're represented as a char type in FIR with a len parameter, and then later on they're expanded into an array of characters when we're translating to the LLVM dialect. However, we don't generate a bounds for these at lowering. The fix in this PR for this is to generate a bounds from the LEN parameter and attatch it to the map on lowering from FIR to the LLVM dialect when we encounter this type. Full diff: https://github.com/llvm/llvm-project/pull/154172.diff 2 Files Affected:
diff --git a/flang/lib/Optimizer/CodeGen/CodeGenOpenMP.cpp b/flang/lib/Optimizer/CodeGen/CodeGenOpenMP.cpp
index 97912bda79b08..1c3fc04779377 100644
--- a/flang/lib/Optimizer/CodeGen/CodeGenOpenMP.cpp
+++ b/flang/lib/Optimizer/CodeGen/CodeGenOpenMP.cpp
@@ -60,6 +60,21 @@ struct MapInfoOpConversion
: public OpenMPFIROpConversion<mlir::omp::MapInfoOp> {
using OpenMPFIROpConversion::OpenMPFIROpConversion;
+ mlir::omp::MapBoundsOp
+ createBoundsForCharString(mlir::ConversionPatternRewriter &rewriter,
+ unsigned int len, mlir::Location loc) const {
+ mlir::Type i64Ty = rewriter.getIntegerType(64);
+ auto lBound = mlir::LLVM::ConstantOp::create(rewriter, loc, i64Ty, 0);
+ auto uBoundAndExt =
+ mlir::LLVM::ConstantOp::create(rewriter, loc, i64Ty, len - 1);
+ auto stride = mlir::LLVM::ConstantOp::create(rewriter, loc, i64Ty, 1);
+ auto baseLb = mlir::LLVM::ConstantOp::create(rewriter, loc, i64Ty, 1);
+ auto mapBoundType = rewriter.getType<mlir::omp::MapBoundsType>();
+ return mlir::omp::MapBoundsOp::create(rewriter, loc, mapBoundType, lBound,
+ uBoundAndExt, uBoundAndExt, stride,
+ /*strideInBytes*/ false, baseLb);
+ }
+
llvm::LogicalResult
matchAndRewrite(mlir::omp::MapInfoOp curOp, OpAdaptor adaptor,
mlir::ConversionPatternRewriter &rewriter) const override {
@@ -69,13 +84,58 @@ struct MapInfoOpConversion
return mlir::failure();
llvm::SmallVector<mlir::NamedAttribute> newAttrs;
- mlir::omp::MapInfoOp newOp;
+ mlir::omp::MapBoundsOp mapBoundsOp;
for (mlir::NamedAttribute attr : curOp->getAttrs()) {
if (auto typeAttr = mlir::dyn_cast<mlir::TypeAttr>(attr.getValue())) {
mlir::Type newAttr;
if (fir::isTypeWithDescriptor(typeAttr.getValue())) {
newAttr = lowerTy().convertBoxTypeAsStruct(
mlir::cast<fir::BaseBoxType>(typeAttr.getValue()));
+ } else if (fir::isa_char_string(fir::unwrapSequenceType(
+ fir::unwrapPassByRefType(typeAttr.getValue()))) &&
+ !characterWithDynamicLen(
+ fir::unwrapPassByRefType(typeAttr.getValue()))) {
+ // Characters with a LEN param are represented as char
+ // arrays/strings, the initial lowering doesn't generate
+ // bounds for these, however, we require them to map the
+ // data appropriately in the later lowering stages. This
+ // is to prevent the need for unecessary caveats
+ // specific to Flang. We also strip the array from the
+ // type so that all variations of strings are treated
+ // identically and there's no caveats or specialisations
+ // required in the later stages. As an example, Boxed
+ // char strings will emit a single char array no matter
+ // the number of dimensions caused by additional array
+ // dimensions which needs specialised for, as it differs
+ // from the non-box variation which will emit each array
+ // wrapping the character array, e.g. given a type of
+ // the same dimensions, if one is boxed, the types would
+ // end up:
+ //
+ // array<i8 x 16>
+ // vs
+ // array<10 x array< 10 x array<i8 x 16>>>
+ //
+ // This means we have to treat one specially in the
+ // lowering. So we try to "canonicalize" it here.
+ // TODO: Handle dynamic LEN characters.
+ if (auto ct = mlir::dyn_cast_or_null<fir::CharacterType>(
+ fir::unwrapSequenceType(typeAttr.getValue()))) {
+ newAttr = converter->convertType(
+ fir::unwrapSequenceType(typeAttr.getValue()));
+ if (auto type = mlir::dyn_cast<mlir::LLVM::LLVMArrayType>(newAttr))
+ newAttr = type.getElementType();
+ // We do not generate for device, as MapBoundsOps are
+ // unsupported, as they're currently unused.
+ auto offloadMod =
+ llvm::dyn_cast_or_null<mlir::omp::OffloadModuleInterface>(
+ *curOp->getParentOfType<mlir::ModuleOp>());
+ if (!offloadMod.getIsTargetDevice())
+ mapBoundsOp = createBoundsForCharString(rewriter, ct.getLen(),
+ curOp.getLoc());
+ } else {
+ newAttr = converter->convertType(typeAttr.getValue());
+ }
} else {
newAttr = converter->convertType(typeAttr.getValue());
}
@@ -85,8 +145,13 @@ struct MapInfoOpConversion
}
}
- rewriter.replaceOpWithNewOp<mlir::omp::MapInfoOp>(
+ auto newOp = rewriter.replaceOpWithNewOp<mlir::omp::MapInfoOp>(
curOp, resTypes, adaptor.getOperands(), newAttrs);
+ if (mapBoundsOp) {
+ rewriter.startOpModification(newOp);
+ newOp.getBoundsMutable().append(mlir::ValueRange{mapBoundsOp});
+ rewriter.finalizeOpModification(newOp);
+ }
return mlir::success();
}
diff --git a/flang/test/Fir/OpenMP/bounds-generation-for-char-arrays.f90 b/flang/test/Fir/OpenMP/bounds-generation-for-char-arrays.f90
new file mode 100644
index 0000000000000..d9d54ee72edb8
--- /dev/null
+++ b/flang/test/Fir/OpenMP/bounds-generation-for-char-arrays.f90
@@ -0,0 +1,96 @@
+// RUN: fir-opt --cfg-conversion --fir-to-llvm-ir="target=aarch64-unknown-linux-gnu" %s | FileCheck %s
+
+module attributes {omp.is_target_device = false} {
+ func.func @_QPchar_array(%arg0 : !fir.ref<!fir.array<10x10x!fir.char<1,16>>>) {
+ %c9 = arith.constant 9 : index
+ %c0 = arith.constant 0 : index
+ %c1 = arith.constant 1 : index
+ %c10 = arith.constant 10 : index
+ %0 = omp.map.bounds lower_bound(%c0 : index) upper_bound(%c9 : index) extent(%c10 : index) stride(%c1 : index) start_idx(%c1 : index)
+ %1 = omp.map.bounds lower_bound(%c0 : index) upper_bound(%c9 : index) extent(%c10 : index) stride(%c1 : index) start_idx(%c1 : index)
+ %2 = omp.map.info var_ptr(%arg0 : !fir.ref<!fir.array<10x10x!fir.char<1,16>>>, !fir.array<10x10x!fir.char<1,16>>) map_clauses(tofrom) capture(ByRef) bounds(%0, %1) -> !fir.ref<!fir.array<10x10x!fir.char<1,16>>> {name = ""}
+ omp.target map_entries(%2 -> %arg1 : !fir.ref<!fir.array<10x10x!fir.char<1,16>>>) {
+ omp.terminator
+ }
+ return
+ }
+
+// CHECK-LABEL: llvm.func @_QPchar_array(
+// CHECK-SAME: %[[ARG0:.*]]: !llvm.ptr) {
+// CHECK: %[[VAL_0:.*]] = llvm.mlir.constant(9 : index) : i64
+// CHECK: %[[VAL_1:.*]] = llvm.mlir.constant(0 : index) : i64
+// CHECK: %[[VAL_2:.*]] = llvm.mlir.constant(1 : index) : i64
+// CHECK: %[[VAL_3:.*]] = llvm.mlir.constant(10 : index) : i64
+// CHECK: %[[VAL_4:.*]] = omp.map.bounds lower_bound(%[[VAL_1]] : i64) upper_bound(%[[VAL_0]] : i64) extent(%[[VAL_3]] : i64) stride(%[[VAL_2]] : i64) start_idx(%[[VAL_2]] : i64)
+// CHECK: %[[VAL_5:.*]] = omp.map.bounds lower_bound(%[[VAL_1]] : i64) upper_bound(%[[VAL_0]] : i64) extent(%[[VAL_3]] : i64) stride(%[[VAL_2]] : i64) start_idx(%[[VAL_2]] : i64)
+// CHECK: %[[VAL_6:.*]] = llvm.mlir.constant(0 : i64) : i64
+// CHECK: %[[VAL_7:.*]] = llvm.mlir.constant(15 : i64) : i64
+// CHECK: %[[VAL_8:.*]] = llvm.mlir.constant(1 : i64) : i64
+// CHECK: %[[VAL_9:.*]] = llvm.mlir.constant(1 : i64) : i64
+// CHECK: %[[VAL_10:.*]] = omp.map.bounds lower_bound(%[[VAL_6]] : i64) upper_bound(%[[VAL_7]] : i64) extent(%[[VAL_7]] : i64) stride(%[[VAL_8]] : i64) start_idx(%[[VAL_9]] : i64)
+// CHECK: %[[VAL_11:.*]] = omp.map.info var_ptr(%[[ARG0]] : !llvm.ptr, i8) map_clauses(tofrom) capture(ByRef) bounds(%[[VAL_4]], %[[VAL_5]], %[[VAL_10]]) -> !llvm.ptr {name = ""}
+// CHECK: omp.target map_entries(%[[VAL_11]] -> %[[VAL_12:.*]] : !llvm.ptr) {
+// CHECK: omp.terminator
+// CHECK: }
+// CHECK: llvm.return
+// CHECK: }
+
+ func.func @_QPallocatable_char_array(%arg0 : !fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>>) {
+ %c1 = arith.constant 1 : index
+ %c0 = arith.constant 0 : index
+ %0 = fir.load %arg0 : !fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>>
+ %1:3 = fir.box_dims %0, %c0 : (!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>, index) -> (index, index, index)
+ %2 = arith.subi %1#1, %c1 : index
+ %3 = omp.map.bounds lower_bound(%c0 : index) upper_bound(%2 : index) extent(%1#1 : index) stride(%1#2 : index) start_idx(%1#0 : index) {stride_in_bytes = true}
+ %4 = arith.muli %1#2, %1#1 : index
+ %5:3 = fir.box_dims %0, %c1 : (!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>, index) -> (index, index, index)
+ %6 = arith.subi %5#1, %c1 : index
+ %7 = omp.map.bounds lower_bound(%c0 : index) upper_bound(%6 : index) extent(%5#1 : index) stride(%4 : index) start_idx(%5#0 : index) {stride_in_bytes = true}
+ %8 = fir.box_offset %arg0 base_addr : (!fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>>) -> !fir.llvm_ptr<!fir.ref<!fir.array<?x?x!fir.char<1,16>>>>
+ %9 = omp.map.info var_ptr(%arg0 : !fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>>, !fir.char<1,16>) map_clauses(tofrom) capture(ByRef) var_ptr_ptr(%8 : !fir.llvm_ptr<!fir.ref<!fir.array<?x?x!fir.char<1,16>>>>) bounds(%3, %7) -> !fir.llvm_ptr<!fir.ref<!fir.array<?x?x!fir.char<1,16>>>> {name = ""}
+ %10 = omp.map.info var_ptr(%arg0 : !fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>>, !fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>) map_clauses(to) capture(ByRef) members(%9 : [0] : !fir.llvm_ptr<!fir.ref<!fir.array<?x?x!fir.char<1,16>>>>) -> !fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>> {name = "csv_chem_list_a"}
+ omp.target map_entries(%10 -> %arg1, %9 -> %arg2 : !fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>>, !fir.llvm_ptr<!fir.ref<!fir.array<?x?x!fir.char<1,16>>>>) {
+ omp.terminator
+ }
+ return
+ }
+
+// CHECK-LABEL: llvm.func @_QPallocatable_char_array(
+// CHECK-SAME: %[[ARG0:.*]]: !llvm.ptr) {
+// CHECK: %[[VAL_0:.*]] = llvm.mlir.constant(1 : i32) : i32
+// CHECK: %[[VAL_1:.*]] = llvm.alloca %[[VAL_0]] x !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<2 x array<3 x i64>>)> {alignment = 8 : i64} : (i32) -> !llvm.ptr
+// CHECK: %[[VAL_2:.*]] = llvm.mlir.constant(1 : index) : i64
+// CHECK: %[[VAL_3:.*]] = llvm.mlir.constant(0 : index) : i64
+// CHECK: %[[VAL_4:.*]] = llvm.mlir.constant(72 : i32) : i32
+// CHECK: "llvm.intr.memcpy"(%[[VAL_1]], %[[ARG0]], %[[VAL_4]]) <{isVolatile = false}> : (!llvm.ptr, !llvm.ptr, i32) -> ()
+// CHECK: %[[VAL_5:.*]] = llvm.getelementptr %[[VAL_1]][0, 7, %[[VAL_3]], 0] : (!llvm.ptr, i64) -> !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<2 x array<3 x i64>>)>
+// CHECK: %[[VAL_6:.*]] = llvm.load %[[VAL_5]] : !llvm.ptr -> i64
+// CHECK: %[[VAL_7:.*]] = llvm.getelementptr %[[VAL_1]][0, 7, %[[VAL_3]], 1] : (!llvm.ptr, i64) -> !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<2 x array<3 x i64>>)>
+// CHECK: %[[VAL_8:.*]] = llvm.load %[[VAL_7]] : !llvm.ptr -> i64
+// CHECK: %[[VAL_9:.*]] = llvm.getelementptr %[[VAL_1]][0, 7, %[[VAL_3]], 2] : (!llvm.ptr, i64) -> !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<2 x array<3 x i64>>)>
+// CHECK: %[[VAL_10:.*]] = llvm.load %[[VAL_9]] : !llvm.ptr -> i64
+// CHECK: %[[VAL_11:.*]] = llvm.sub %[[VAL_8]], %[[VAL_2]] : i64
+// CHECK: %[[VAL_12:.*]] = omp.map.bounds lower_bound(%[[VAL_3]] : i64) upper_bound(%[[VAL_11]] : i64) extent(%[[VAL_8]] : i64) stride(%[[VAL_10]] : i64) start_idx(%[[VAL_6]] : i64) {stride_in_bytes = true}
+// CHECK: %[[VAL_13:.*]] = llvm.mul %[[VAL_10]], %[[VAL_8]] : i64
+// CHECK: %[[VAL_14:.*]] = llvm.getelementptr %[[VAL_1]][0, 7, %[[VAL_2]], 0] : (!llvm.ptr, i64) -> !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<2 x array<3 x i64>>)>
+// CHECK: %[[VAL_15:.*]] = llvm.load %[[VAL_14]] : !llvm.ptr -> i64
+// CHECK: %[[VAL_16:.*]] = llvm.getelementptr %[[VAL_1]][0, 7, %[[VAL_2]], 1] : (!llvm.ptr, i64) -> !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<2 x array<3 x i64>>)>
+// CHECK: %[[VAL_17:.*]] = llvm.load %[[VAL_16]] : !llvm.ptr -> i64
+// CHECK: %[[VAL_18:.*]] = llvm.getelementptr %[[VAL_1]][0, 7, %[[VAL_2]], 2] : (!llvm.ptr, i64) -> !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<2 x array<3 x i64>>)>
+// CHECK: %[[VAL_19:.*]] = llvm.load %[[VAL_18]] : !llvm.ptr -> i64
+// CHECK: %[[VAL_20:.*]] = llvm.sub %[[VAL_17]], %[[VAL_2]] : i64
+// CHECK: %[[VAL_21:.*]] = omp.map.bounds lower_bound(%[[VAL_3]] : i64) upper_bound(%[[VAL_20]] : i64) extent(%[[VAL_17]] : i64) stride(%[[VAL_13]] : i64) start_idx(%[[VAL_15]] : i64) {stride_in_bytes = true}
+// CHECK: %[[VAL_22:.*]] = llvm.getelementptr %[[ARG0]][0, 0] : (!llvm.ptr) -> !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<2 x array<3 x i64>>)>
+// CHECK: %[[VAL_23:.*]] = llvm.mlir.constant(0 : i64) : i64
+// CHECK: %[[VAL_24:.*]] = llvm.mlir.constant(15 : i64) : i64
+// CHECK: %[[VAL_25:.*]] = llvm.mlir.constant(1 : i64) : i64
+// CHECK: %[[VAL_26:.*]] = llvm.mlir.constant(1 : i64) : i64
+// CHECK: %[[VAL_27:.*]] = omp.map.bounds lower_bound(%[[VAL_23]] : i64) upper_bound(%[[VAL_24]] : i64) extent(%[[VAL_24]] : i64) stride(%[[VAL_25]] : i64) start_idx(%[[VAL_26]] : i64)
+// CHECK: %[[VAL_28:.*]] = omp.map.info var_ptr(%[[ARG0]] : !llvm.ptr, i8) map_clauses(tofrom) capture(ByRef) var_ptr_ptr(%[[VAL_22]] : !llvm.ptr) bounds(%[[VAL_12]], %[[VAL_21]], %[[VAL_27]]) -> !llvm.ptr {name = ""}
+// CHECK: %[[VAL_29:.*]] = omp.map.info var_ptr(%[[ARG0]] : !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<2 x array<3 x i64>>)>) map_clauses(to) capture(ByRef) members(%[[VAL_28]] : [0] : !llvm.ptr) -> !llvm.ptr {name = "csv_chem_list_a"}
+// CHECK: omp.target map_entries(%[[VAL_29]] -> %[[VAL_30:.*]], %[[VAL_28]] -> %[[VAL_31:.*]] : !llvm.ptr, !llvm.ptr) {
+// CHECK: omp.terminator
+// CHECK: }
+// CHECK: llvm.return
+// CHECK: }
+}
|
|
Small ping for reviewer attention on this PR if at all possible please :-)! |
| // dimensions which needs specialised for, as it differs | ||
| // from the non-box variation which will emit each array | ||
| // wrapping the character array, e.g. given a type of | ||
| // the same dimensions, if one is boxed, the types would |
There was a problem hiding this comment.
I don't understand the 2nd part of this paragraph. I tried the following code and got 2 omp.map.bounds. One with 2 elements and other with 10 elements which sort of made sense. But I don't understand what you mean by the additional array dimensions. May be some example fortran code will help.
character(len=10), dimension(2) :: keys
There was a problem hiding this comment.
I agree with @abidh - Even I am having trouble understanding this. Could you please elaborate with an example?
There was a problem hiding this comment.
So, given a type like the following:
CHARACTER(LEN=16), dimension(:,:), allocatable :: char_arr
When we map it we'll emit something like the below for the base address, that has two bounds:
%17 = omp.map.bounds lower_bound(%c0 : index) upper_bound(%16 : index) extent(%15#1 : index) stride(%15#2 : index) start_idx(%14#0 : index) {stride_in_bytes = true}
...
%23 = omp.map.bounds lower_bound(%c0 : index) upper_bound(%22 : index) extent(%21#1 : index) stride(%18 : index) start_idx(%20#0 : index) {stride_in_bytes = true}
%24 = fir.box_offset %2 base_addr : (!fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>>) -> !fir.llvm_ptr<!fir.ref<!fir.array<?x?x!fir.char<1,16>>>>
%25 = omp.map.info var_ptr(%2 : !fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.char<1,16>>>>>, !fir.char<1,16>) map_clauses(exit_release_or_enter_alloc) capture(ByRef) var_ptr_ptr(%24 : !fir.llvm_ptr<!fir.ref<!fir.array<?x?x!fir.char<1,16>>>>) bounds(%17, %23) -> !fir.llvm_ptr<!fir.ref<!fir.array<?x?x!fir.char<1,16>>>> {name = ""}
...
The full fir type is:
!fir.array<?x?x!fir.char<1,16>
Which we currently peel off in lowering ourselves to:
!fir.char<1,16>
Which converted to the LLVM dialect becomes:
!llvm.array<16 x i8>
However, even if we didn't peel off the dynamic array extents, the lowering to LLVM dialect does that in any case, so we would end up with the same type (and problem) regardless.
So we devolve what is effectively a 3-D array to 1-D, and now we only have 2 bounds, the bounds we're missing in this case is the fir.char<1,16> which expands to the innermost array.
Another scenario occurs for:
CHARACTER(LEN=16), dimension(10,10) :: char_arr
Where we end up with:
%4 = omp.map.bounds lower_bound(%c0 : index) upper_bound(%c9 : index) extent(%c10 : index) stride(%c1 : index) start_idx(%c1 : index)
%5 = omp.map.bounds lower_bound(%c0 : index) upper_bound(%c9 : index) extent(%c10 : index) stride(%c1 : index) start_idx(%c1 : index)
%6 = omp.map.info var_ptr(%3 : !fir.ref<!fir.array<10x10x!fir.char<1,16>>>, !fir.array<10x10x!fir.char<1,16>>) map_clauses(exit_release_or_enter_alloc) capture(ByRef) bounds(%4, %5) -> !fir.ref<!fir.array<10x10x!fir.char<1,16>>> {name = "char_arr"}
Where the fir type is:
!fir.array<10x10x!fir.char<1,16>>
And we end up with an LLVM dialect conversion of:
llvm.array<10 x array<10 x array<16 x i8>>
In this case, we actually keep the full type, but have generated two bounds for it, and neither are the innermost. But in this particular case as it's constant we could in theory calculate the size from the type, but we don't do that at the moment, future work.
But now we have two different cases for something that we can treat identically or even the same as every other array (which is the goal of this PR), having to be special cased in the later lowering due to the FIR dialect conversion.
The 1-D variation of this also follows a similar trend, from what I can tell, at least with the build I have just now, it's worth noting that downstream already has this patch applied for us.
So what we do in this PR is try to "canonicalize" the type in both of these cases and generate the additional bound necessary to calculate sizes and offsets where necessary. This lets us avoid special casing the lowering. In theory, we could handle this in OpenMPToLLVMIRTranslation, but special casing for the multiple variations of fortran character arrays doesn't seem like the way to go.
A third alternative to this could be to teach the frontend to generate bounds for this type on initial lowering, but I don't like the idea of special treatment of the type for Fortran and ferrying it around everywhere, it might be confusing to some, I'd personal prefer to just generate it at the stage we're going to require it! And it also doesn't solve the inconsistency of the type lowering for these between FIR -> LLVM dialect. But I'm open to the idea of course (and any alternatives in general :-) ).
Hopefully this and the extended comment helps explain things a little bit better, I've added some runtime tests for this that should fail without this PR, as well as expanded the comment as requested and tried to make it read a little more sanely! Also happy to talk about it in a call if that helps at all as well!
There was a problem hiding this comment.
I think I understand now. What you are saying is that essentially, in the case of strings (CHARACTER with LEN parameter), we cannot generalize/unify the translation of an n-dimensional array of bytes (i8 or chars) which is also a n-1 dimensional array of CHARACTERS with LEN parameters because the innermost (constant) size/bound is baked into the type but not pulled out as a bound.
So we dive into the fir.char<> type and pull out the constant as a bound so that the translation can now be simplified by generalizing it to the n-dimensional mapping of i8 types, correct?
There was a problem hiding this comment.
That is correct! Trying to avoid complicating the later lowering stage further specifically for Fortran essentially by addressing it here (I did try having it in the lowering stages in an initial implementation, it works, but it's a bit messy and doesn't seem the right place to do it) :-) Thank you for the review!
agozillon
force-pushed
the
char-array-omp-map-fixes
branch
from
debdcff to
8b68359
Compare
|
Updated the PR to elaborate further on the comments and add two additional runtime tests this PR aims to help address, also left a little comment to try to explain the reasoning/idea behind the addition a bit more, hopefully it's helpful! char-array-map-test-v1.f90 char-array-map-test-v2.f90 |
|
Sorry everyone, just the usual ping to ask for a little reviewer attention if that's at all possible, thank you all very much! :-) |
Sorry @agozillon - i had been preoccupied with something else and I completely missed your reply to my question from last week. I am winding down for the day right now but I will look at this first thing Friday morning. |
|
Thank you very much for the review @bhandarkar-pranav and @abidh! Do my modifications and added information help clarify things a bit better @abidh ? Happy to try and explain my thought process a bit further if it helps at all :-) |
There was a problem hiding this comment.
Thanks for the explanation and the tests. That helps to understand the issue better. My initial confusion also stems from the fact that debugging of string variables worked correctly in target region without this change. But it has different source for bounds info.
| if (auto type = mlir::dyn_cast<mlir::LLVM::LLVMArrayType>(newAttr)) | ||
| newAttr = type.getElementType(); | ||
| // We do not generate for device, as MapBoundsOps are | ||
| // unsupported, as they're currently unused. |
There was a problem hiding this comment.
This comment probably should be rephrased. It is not clear if they are unsupported or unused or both.
There was a problem hiding this comment.
Both, we omit a lot of unused host operations from the device MLIR now and this is one of those :-) Thank you for pointing it out, I'll try to rephrase it better before I land this PR!
|
Thank you both again for the review! I'll try to land this PR after the final minor update of the comment in the next day or so :-) |
Currently, there's a number of issues with mapping characters with LEN's specified (strings effectively). They're represented as a char type in FIR with a len parameter, and then later on they're expanded into an array of characters when we're translating to the LLVM dialect. However, we don't generate a bounds for these at lowering. The fix in this PR for this is to generate a bounds from the LEN parameter and attatch it to the map on lowering from FIR to the LLVM dialect when we encounter this type.
agozillon
force-pushed
the
char-array-omp-map-fixes
branch
from
8b68359 to
a6bcb51
Compare
Currently, there's a number of issues with mapping characters with LEN's specified (strings effectively). They're represented as a char type in FIR with a len parameter, and then later on they're expanded into an array of characters when we're translating to the LLVM dialect. However, we don't generate a bounds for these at lowering. The fix in this PR for this is to generate a bounds from the LEN parameter and attatch it to the map on lowering from FIR to the LLVM dialect when we encounter this type.