Skip to content

Commit 2dc17fd

Browse files
jeanPerierjeanPerier
authored
[flang] fix isSimplyContiguous and isOptional hlfir::Entity methods (llvm#125215)
Fix isSimplyContiguous: - It ignored scalars, causing scalar fir.box to not be opened when possible in `translateToExtendedValue` Fix isOptional: It is not reliable when the memory SSA value cannot be linked to a declare. This is exposed by the `isSimplyContiguous` fix, This is wrong because declare operation should not always assumed to be visible (e.g., value may travel through a select, or the optional be generated by the compiler like genOptionalBox in lib/Lower/ConvertCall.cpp). - Turn `isOptional` into a safer `mayBeOptional` - Update translateToExtendedValue to open scalar fir.box for such values in a fir.if. - It turned out some `translateToExtendedValue` usage relied on fir.box of optional scalars to be left untouched (mainly because they want to forward those fir.box to the runtime), add an option to allow that.
1 parent f3c4b58 commit 2dc17fd

File tree

5 files changed

+196
-14
lines changed

5 files changed

+196
-14
lines changed

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

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ class Entity : public mlir::Value {
125125
bool isSimplyContiguous() const {
126126
// If this can be described without a fir.box in FIR, this must
127127
// be contiguous.
128-
if (!hlfir::isBoxAddressOrValueType(getFirBase().getType()))
128+
if (!hlfir::isBoxAddressOrValueType(getFirBase().getType()) || isScalar())
129129
return true;
130130
// Otherwise, if this entity has a visible declaration in FIR,
131131
// or is the dereference of an allocatable or contiguous pointer
@@ -150,10 +150,7 @@ class Entity : public mlir::Value {
150150
return base.getDefiningOp<fir::FortranVariableOpInterface>();
151151
}
152152

153-
bool isOptional() const {
154-
auto varIface = getIfVariableInterface();
155-
return varIface ? varIface.isOptional() : false;
156-
}
153+
bool mayBeOptional() const;
157154

158155
bool isParameter() const {
159156
auto varIface = getIfVariableInterface();
@@ -210,7 +207,8 @@ class EntityWithAttributes : public Entity {
210207
using CleanupFunction = std::function<void()>;
211208
std::pair<fir::ExtendedValue, std::optional<CleanupFunction>>
212209
translateToExtendedValue(mlir::Location loc, fir::FirOpBuilder &builder,
213-
Entity entity, bool contiguousHint = false);
210+
Entity entity, bool contiguousHint = false,
211+
bool keepScalarOptionalBoxed = false);
214212

215213
/// Function to translate FortranVariableOpInterface to fir::ExtendedValue.
216214
/// It may generates IR to unbox fir.boxchar, but has otherwise no side effects

‎flang/lib/Optimizer/Builder/HLFIRTools.cpp‎

Lines changed: 108 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,25 @@ bool hlfir::Entity::mayHaveNonDefaultLowerBounds() const {
221221
return true;
222222
}
223223

224+
mlir::Operation *traverseConverts(mlir::Operation *op) {
225+
while (auto convert = llvm::dyn_cast_or_null<fir::ConvertOp>(op))
226+
op = convert.getValue().getDefiningOp();
227+
return op;
228+
}
229+
230+
bool hlfir::Entity::mayBeOptional() const {
231+
if (!isVariable())
232+
return false;
233+
// TODO: introduce a fir type to better identify optionals.
234+
if (mlir::Operation *op = traverseConverts(getDefiningOp())) {
235+
if (auto varIface = llvm::dyn_cast<fir::FortranVariableOpInterface>(op))
236+
return varIface.isOptional();
237+
return !llvm::isa<fir::AllocaOp, fir::AllocMemOp, fir::ReboxOp,
238+
fir::EmboxOp, fir::LoadOp>(op);
239+
}
240+
return true;
241+
}
242+
224243
fir::FortranVariableOpInterface
225244
hlfir::genDeclare(mlir::Location loc, fir::FirOpBuilder &builder,
226245
const fir::ExtendedValue &exv, llvm::StringRef name,
@@ -963,9 +982,69 @@ llvm::SmallVector<mlir::Value> hlfir::genLoopNestWithReductions(
963982
return outerLoop->getResults();
964983
}
965984

985+
template <typename Lambda>
986+
static fir::ExtendedValue
987+
conditionallyEvaluate(mlir::Location loc, fir::FirOpBuilder &builder,
988+
mlir::Value condition, const Lambda &genIfTrue) {
989+
mlir::OpBuilder::InsertPoint insertPt = builder.saveInsertionPoint();
990+
991+
// Evaluate in some region that will be moved into the actual ifOp (the actual
992+
// ifOp can only be created when the result types are known).
993+
auto badIfOp = builder.create<fir::IfOp>(loc, condition.getType(), condition,
994+
/*withElseRegion=*/false);
995+
mlir::Block *preparationBlock = &badIfOp.getThenRegion().front();
996+
builder.setInsertionPointToStart(preparationBlock);
997+
fir::ExtendedValue result = genIfTrue();
998+
fir::ResultOp resultOp = result.match(
999+
[&](const fir::CharBoxValue &box) -> fir::ResultOp {
1000+
return builder.create<fir::ResultOp>(
1001+
loc, mlir::ValueRange{box.getAddr(), box.getLen()});
1002+
},
1003+
[&](const mlir::Value &addr) -> fir::ResultOp {
1004+
return builder.create<fir::ResultOp>(loc, addr);
1005+
},
1006+
[&](const auto &) -> fir::ResultOp {
1007+
TODO(loc, "unboxing non scalar optional fir.box");
1008+
});
1009+
builder.restoreInsertionPoint(insertPt);
1010+
1011+
// Create actual fir.if operation.
1012+
auto ifOp =
1013+
builder.create<fir::IfOp>(loc, resultOp->getOperandTypes(), condition,
1014+
/*withElseRegion=*/true);
1015+
// Move evaluation into Then block,
1016+
preparationBlock->moveBefore(&ifOp.getThenRegion().back());
1017+
ifOp.getThenRegion().back().erase();
1018+
// Create absent result in the Else block.
1019+
builder.setInsertionPointToStart(&ifOp.getElseRegion().front());
1020+
llvm::SmallVector<mlir::Value> absentValues;
1021+
for (mlir::Type resTy : ifOp->getResultTypes()) {
1022+
if (fir::isa_ref_type(resTy) || fir::isa_box_type(resTy))
1023+
absentValues.emplace_back(builder.create<fir::AbsentOp>(loc, resTy));
1024+
else
1025+
absentValues.emplace_back(builder.create<fir::ZeroOp>(loc, resTy));
1026+
}
1027+
builder.create<fir::ResultOp>(loc, absentValues);
1028+
badIfOp->erase();
1029+
1030+
// Build fir::ExtendedValue from the result values.
1031+
builder.setInsertionPointAfter(ifOp);
1032+
return result.match(
1033+
[&](const fir::CharBoxValue &box) -> fir::ExtendedValue {
1034+
return fir::CharBoxValue{ifOp.getResult(0), ifOp.getResult(1)};
1035+
},
1036+
[&](const mlir::Value &) -> fir::ExtendedValue {
1037+
return ifOp.getResult(0);
1038+
},
1039+
[&](const auto &) -> fir::ExtendedValue {
1040+
TODO(loc, "unboxing non scalar optional fir.box");
1041+
});
1042+
}
1043+
9661044
static fir::ExtendedValue translateVariableToExtendedValue(
9671045
mlir::Location loc, fir::FirOpBuilder &builder, hlfir::Entity variable,
968-
bool forceHlfirBase = false, bool contiguousHint = false) {
1046+
bool forceHlfirBase = false, bool contiguousHint = false,
1047+
bool keepScalarOptionalBoxed = false) {
9691048
assert(variable.isVariable() && "must be a variable");
9701049
// When going towards FIR, use the original base value to avoid
9711050
// introducing descriptors at runtime when they are not required.
@@ -984,14 +1063,33 @@ static fir::ExtendedValue translateVariableToExtendedValue(
9841063
const bool contiguous = variable.isSimplyContiguous() || contiguousHint;
9851064
const bool isAssumedRank = variable.isAssumedRank();
9861065
if (!contiguous || variable.isPolymorphic() ||
987-
variable.isDerivedWithLengthParameters() || variable.isOptional() ||
988-
isAssumedRank) {
1066+
variable.isDerivedWithLengthParameters() || isAssumedRank) {
9891067
llvm::SmallVector<mlir::Value> nonDefaultLbounds;
9901068
if (!isAssumedRank)
9911069
nonDefaultLbounds = getNonDefaultLowerBounds(loc, builder, variable);
9921070
return fir::BoxValue(base, nonDefaultLbounds,
9931071
getExplicitTypeParams(variable));
9941072
}
1073+
if (variable.mayBeOptional()) {
1074+
if (!keepScalarOptionalBoxed && variable.isScalar()) {
1075+
mlir::Value isPresent = builder.create<fir::IsPresentOp>(
1076+
loc, builder.getI1Type(), variable);
1077+
return conditionallyEvaluate(
1078+
loc, builder, isPresent, [&]() -> fir::ExtendedValue {
1079+
mlir::Value base = genVariableRawAddress(loc, builder, variable);
1080+
if (variable.isCharacter()) {
1081+
mlir::Value len =
1082+
genCharacterVariableLength(loc, builder, variable);
1083+
return fir::CharBoxValue{base, len};
1084+
}
1085+
return base;
1086+
});
1087+
}
1088+
llvm::SmallVector<mlir::Value> nonDefaultLbounds =
1089+
getNonDefaultLowerBounds(loc, builder, variable);
1090+
return fir::BoxValue(base, nonDefaultLbounds,
1091+
getExplicitTypeParams(variable));
1092+
}
9951093
// Otherwise, the variable can be represented in a fir::ExtendedValue
9961094
// without the overhead of a fir.box.
9971095
base = genVariableRawAddress(loc, builder, variable);
@@ -1035,10 +1133,12 @@ hlfir::translateToExtendedValue(mlir::Location loc, fir::FirOpBuilder &builder,
10351133

10361134
std::pair<fir::ExtendedValue, std::optional<hlfir::CleanupFunction>>
10371135
hlfir::translateToExtendedValue(mlir::Location loc, fir::FirOpBuilder &builder,
1038-
hlfir::Entity entity, bool contiguousHint) {
1136+
hlfir::Entity entity, bool contiguousHint,
1137+
bool keepScalarOptionalBoxed) {
10391138
if (entity.isVariable())
10401139
return {translateVariableToExtendedValue(loc, builder, entity, false,
1041-
contiguousHint),
1140+
contiguousHint,
1141+
keepScalarOptionalBoxed),
10421142
std::nullopt};
10431143

10441144
if (entity.isProcedure()) {
@@ -1094,7 +1194,9 @@ hlfir::convertToBox(mlir::Location loc, fir::FirOpBuilder &builder,
10941194
if (entity.isProcedurePointer())
10951195
entity = hlfir::derefPointersAndAllocatables(loc, builder, entity);
10961196

1097-
auto [exv, cleanup] = translateToExtendedValue(loc, builder, entity);
1197+
auto [exv, cleanup] =
1198+
translateToExtendedValue(loc, builder, entity, /*contiguousHint=*/false,
1199+
/*keepScalarOptionalBoxed=*/true);
10981200
// Procedure entities should not go through createBoxValue that embox
10991201
// object entities. Return the fir.boxproc directly.
11001202
if (entity.isProcedure())

‎flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIRIntrinsics.cpp‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,8 +121,14 @@ class HlfirIntrinsicConversion : public mlir::OpRewritePattern<OP> {
121121
// simplified since the fir.box lowered here are now guarenteed to
122122
// contain the local lower bounds thanks to the hlfir.declare (the extra
123123
// rebox can be removed).
124-
auto [exv, cleanup] =
125-
hlfir::translateToExtendedValue(loc, builder, entity);
124+
// When taking arguments as descriptors, the runtime expect absent
125+
// OPTIONAL to be a nullptr to a descriptor, lowering has already
126+
// prepared such descriptors as needed, hence set
127+
// keepScalarOptionalBoxed to avoid building descriptors with a null
128+
// address for them.
129+
auto [exv, cleanup] = hlfir::translateToExtendedValue(
130+
loc, builder, entity, /*contiguous=*/false,
131+
/*keepScalarOptionalBoxed=*/true);
126132
if (cleanup)
127133
cleanupFns.push_back(*cleanup);
128134
ret.emplace_back(exv);

‎flang/test/HLFIR/assign-codegen.fir‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -427,3 +427,57 @@ func.func @test_upoly_expr_assignment(%arg0: !fir.class<!fir.array<?xnone>> {fir
427427
// CHECK: }
428428
// CHECK: return
429429
// CHECK: }
430+
431+
func.func @test_scalar_box(%arg0: f32, %arg1: !fir.box<!fir.ptr<f32>>) {
432+
%x = fir.declare %arg1 {uniq_name = "x"} : (!fir.box<!fir.ptr<f32>>) -> !fir.box<!fir.ptr<f32>>
433+
hlfir.assign %arg0 to %x : f32, !fir.box<!fir.ptr<f32>>
434+
return
435+
}
436+
// CHECK-LABEL: func.func @test_scalar_box(
437+
// CHECK-SAME: %[[VAL_0:.*]]: f32,
438+
// CHECK-SAME: %[[VAL_1:.*]]: !fir.box<!fir.ptr<f32>>) {
439+
// CHECK: %[[VAL_2:.*]] = fir.declare %[[VAL_1]] {uniq_name = "x"} : (!fir.box<!fir.ptr<f32>>) -> !fir.box<!fir.ptr<f32>>
440+
// CHECK: %[[VAL_3:.*]] = fir.box_addr %[[VAL_2]] : (!fir.box<!fir.ptr<f32>>) -> !fir.ptr<f32>
441+
// CHECK: fir.store %[[VAL_0]] to %[[VAL_3]] : !fir.ptr<f32>
442+
443+
func.func @test_scalar_opt_box(%arg0: f32, %arg1: !fir.box<!fir.ptr<f32>>) {
444+
%x = fir.declare %arg1 {fortran_attrs = #fir.var_attrs<optional>, uniq_name = "x"} : (!fir.box<!fir.ptr<f32>>) -> !fir.box<!fir.ptr<f32>>
445+
hlfir.assign %arg0 to %x : f32, !fir.box<!fir.ptr<f32>>
446+
return
447+
}
448+
// CHECK-LABEL: func.func @test_scalar_opt_box(
449+
// CHECK-SAME: %[[VAL_0:.*]]: f32,
450+
// CHECK-SAME: %[[VAL_1:.*]]: !fir.box<!fir.ptr<f32>>) {
451+
// CHECK: %[[VAL_2:.*]] = fir.declare %[[VAL_1]] {fortran_attrs = #fir.var_attrs<optional>, uniq_name = "x"} : (!fir.box<!fir.ptr<f32>>) -> !fir.box<!fir.ptr<f32>>
452+
// CHECK: %[[VAL_3:.*]] = fir.is_present %[[VAL_2]] : (!fir.box<!fir.ptr<f32>>) -> i1
453+
// CHECK: %[[VAL_4:.*]] = fir.if %[[VAL_3]] -> (!fir.ptr<f32>) {
454+
// CHECK: %[[VAL_5:.*]] = fir.box_addr %[[VAL_2]] : (!fir.box<!fir.ptr<f32>>) -> !fir.ptr<f32>
455+
// CHECK: fir.result %[[VAL_5]] : !fir.ptr<f32>
456+
// CHECK: } else {
457+
// CHECK: %[[VAL_6:.*]] = fir.absent !fir.ptr<f32>
458+
// CHECK: fir.result %[[VAL_6]] : !fir.ptr<f32>
459+
// CHECK: }
460+
// CHECK: fir.store %[[VAL_0]] to %[[VAL_4]] : !fir.ptr<f32>
461+
462+
func.func @test_scalar_opt_char_box(%arg0: !fir.ref<!fir.char<1,10>>, %arg1: !fir.box<!fir.char<1,?>>) {
463+
%x = fir.declare %arg1 {fortran_attrs = #fir.var_attrs<optional>, uniq_name = "x"} : (!fir.box<!fir.char<1,?>>) -> !fir.box<!fir.char<1,?>>
464+
hlfir.assign %arg0 to %x : !fir.ref<!fir.char<1,10>>, !fir.box<!fir.char<1,?>>
465+
return
466+
}
467+
// CHECK-LABEL: func.func @test_scalar_opt_char_box(
468+
// CHECK-SAME: %[[VAL_0:.*]]: !fir.ref<!fir.char<1,10>>,
469+
// CHECK-SAME: %[[VAL_1:.*]]: !fir.box<!fir.char<1,?>>) {
470+
// CHECK: %[[VAL_2:.*]] = fir.declare %[[VAL_1]] {fortran_attrs = #fir.var_attrs<optional>, uniq_name = "x"} : (!fir.box<!fir.char<1,?>>) -> !fir.box<!fir.char<1,?>>
471+
// CHECK: %[[VAL_3:.*]] = arith.constant 10 : index
472+
// CHECK: %[[VAL_4:.*]] = fir.is_present %[[VAL_2]] : (!fir.box<!fir.char<1,?>>) -> i1
473+
// CHECK: %[[VAL_5:.*]]:2 = fir.if %[[VAL_4]] -> (!fir.ref<!fir.char<1,?>>, index) {
474+
// CHECK: %[[VAL_6:.*]] = fir.box_addr %[[VAL_2]] : (!fir.box<!fir.char<1,?>>) -> !fir.ref<!fir.char<1,?>>
475+
// CHECK: %[[VAL_7:.*]] = fir.box_elesize %[[VAL_2]] : (!fir.box<!fir.char<1,?>>) -> index
476+
// CHECK: fir.result %[[VAL_6]], %[[VAL_7]] : !fir.ref<!fir.char<1,?>>, index
477+
// CHECK: } else {
478+
// CHECK: %[[VAL_8:.*]] = fir.absent !fir.ref<!fir.char<1,?>>
479+
// CHECK: %[[VAL_9:.*]] = fir.zero_bits index
480+
// CHECK: fir.result %[[VAL_8]], %[[VAL_9]] : !fir.ref<!fir.char<1,?>>, index
481+
// CHECK: }
482+
// ...
483+
// CHECK: fir.call @llvm.memmove.p0.p0.i64(

‎flang/test/HLFIR/maxval-lowering.fir‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,3 +216,25 @@ func.func @_QPmaxval6(%arg0: !fir.box<!fir.array<?x!fir.char<1,?>>> {fir.bindc_n
216216
// CHECK: hlfir.destroy %[[ASEXPR]]
217217
// CHECK-NEXT: return
218218
// CHECK-NEXT: }
219+
220+
func.func @_QPmaxval_opt_mask(%arg0: !fir.box<!fir.array<?x?xf32>> {fir.bindc_name = "input"}, %arg1: !fir.ref<!fir.logical<4>> {fir.bindc_name = "mask", fir.optional}) -> f32 {
221+
%0 = fir.dummy_scope : !fir.dscope
222+
%1:2 = hlfir.declare %arg0 dummy_scope %0 {fortran_attrs = #fir.var_attrs<intent_in>, uniq_name = "_QFmaxval_opt_maskEinput"} : (!fir.box<!fir.array<?x?xf32>>, !fir.dscope) -> (!fir.box<!fir.array<?x?xf32>>, !fir.box<!fir.array<?x?xf32>>)
223+
%2:2 = hlfir.declare %arg1 dummy_scope %0 {fortran_attrs = #fir.var_attrs<intent_in, optional>, uniq_name = "_QFmaxval_opt_maskEmask"} : (!fir.ref<!fir.logical<4>>, !fir.dscope) -> (!fir.ref<!fir.logical<4>>, !fir.ref<!fir.logical<4>>)
224+
%3 = fir.alloca f32 {bindc_name = "maxval_1", uniq_name = "_QFmaxval_opt_maskEmaxval_1"}
225+
%4:2 = hlfir.declare %3 {uniq_name = "_QFmaxval_opt_maskEmaxval_1"} : (!fir.ref<f32>) -> (!fir.ref<f32>, !fir.ref<f32>)
226+
%5 = fir.is_present %2#0 : (!fir.ref<!fir.logical<4>>) -> i1
227+
%6 = fir.embox %2#1 : (!fir.ref<!fir.logical<4>>) -> !fir.box<!fir.logical<4>>
228+
%7 = fir.absent !fir.box<!fir.logical<4>>
229+
%8 = arith.select %5, %6, %7 : !fir.box<!fir.logical<4>>
230+
%9 = hlfir.maxval %1#0 mask %8 : (!fir.box<!fir.array<?x?xf32>>, !fir.box<!fir.logical<4>>) -> f32
231+
hlfir.assign %9 to %4#0 : f32, !fir.ref<f32>
232+
%10 = fir.load %4#1 : !fir.ref<f32>
233+
return %10 : f32
234+
}
235+
// CHECK-LABEL: func.func @_QPmaxval_opt_mask(
236+
// CHECK: %[[VAL_10:.*]] = fir.embox %{{.*}} : (!fir.ref<!fir.logical<4>>) -> !fir.box<!fir.logical<4>>
237+
// CHECK: %[[VAL_11:.*]] = fir.absent !fir.box<!fir.logical<4>>
238+
// CHECK: %[[VAL_12:.*]] = arith.select %{{.*}}, %[[VAL_10]], %[[VAL_11]] : !fir.box<!fir.logical<4>>
239+
// CHECK: %[[VAL_17:.*]] = fir.convert %[[VAL_12]] : (!fir.box<!fir.logical<4>>) -> !fir.box<none>
240+
// CHECK: %[[VAL_18:.*]] = fir.call @_FortranAMaxvalReal4(%{{.*}}, %{{.*}}, %{{.*}}, %{{.*}}, %{{.*}}) : (!fir.box<none>, !fir.ref<i8>, i32, i32, !fir.box<none>) -> f32

0 commit comments

Comments
 (0)