[flang-commits] [flang] [flang] fix regression with optional after PR125059 (PR #125215)
via flang-commits
flang-commits at lists.llvm.org
Fri Jan 31 04:54:46 PST 2025
https://github.com/jeanPerier created https://github.com/llvm/llvm-project/pull/125215
Some gfortran and Fujistu tests started failing after PR125059 (https://linaro.atlassian.net/browse/LLVM-1538).
The reason is that variable.isOptional() is not reliable for when used in translateToExtendedValue on SSA values that have been built for intrinsic argument (mainly because with `asBox`, optional fir.box may be compiler generated [here in genOptionalBox](https://github.com/llvm/llvm-project/blob/9cf8ee91451788b08a0bcda924bfbb3754c454da/flang/lib/Lower/ConvertCall.cpp#L1831) and do not have associated fir.declare). Adding a declare would be possible, but it is heavy, and in general, not finding a declare should not be a free pass to assume something is not OPTIONAL (I really need to update FIR types here).
Hence:
- 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.
>From b8b72cde4b0deff7f56b0c25f42df0f589e57bd0 Mon Sep 17 00:00:00 2001
From: Jean Perier <jperier at nvidia.com>
Date: Fri, 31 Jan 2025 01:58:04 -0800
Subject: [PATCH] [flang] fix regression with optional after PR125059
---
.../flang/Optimizer/Builder/HLFIRTools.h | 8 +-
flang/lib/Optimizer/Builder/HLFIRTools.cpp | 106 +++++++++++++++++-
.../HLFIR/Transforms/LowerHLFIRIntrinsics.cpp | 10 +-
flang/test/HLFIR/assign-codegen.fir | 50 ++++++++-
4 files changed, 160 insertions(+), 14 deletions(-)
diff --git a/flang/include/flang/Optimizer/Builder/HLFIRTools.h b/flang/include/flang/Optimizer/Builder/HLFIRTools.h
index d8785969bb7247..8b1235b50cc6fb 100644
--- a/flang/include/flang/Optimizer/Builder/HLFIRTools.h
+++ b/flang/include/flang/Optimizer/Builder/HLFIRTools.h
@@ -150,10 +150,7 @@ class Entity : public mlir::Value {
return base.getDefiningOp<fir::FortranVariableOpInterface>();
}
- bool isOptional() const {
- auto varIface = getIfVariableInterface();
- return varIface ? varIface.isOptional() : false;
- }
+ bool mayBeOptional() const;
bool isParameter() const {
auto varIface = getIfVariableInterface();
@@ -210,7 +207,8 @@ class EntityWithAttributes : public Entity {
using CleanupFunction = std::function<void()>;
std::pair<fir::ExtendedValue, std::optional<CleanupFunction>>
translateToExtendedValue(mlir::Location loc, fir::FirOpBuilder &builder,
- Entity entity, bool contiguousHint = false);
+ Entity entity, bool contiguousHint = false,
+ bool keepScalarOptionalBoxed = false);
/// Function to translate FortranVariableOpInterface to fir::ExtendedValue.
/// It may generates IR to unbox fir.boxchar, but has otherwise no side effects
diff --git a/flang/lib/Optimizer/Builder/HLFIRTools.cpp b/flang/lib/Optimizer/Builder/HLFIRTools.cpp
index f71adf123511d5..63a02677fa1e88 100644
--- a/flang/lib/Optimizer/Builder/HLFIRTools.cpp
+++ b/flang/lib/Optimizer/Builder/HLFIRTools.cpp
@@ -221,6 +221,24 @@ bool hlfir::Entity::mayHaveNonDefaultLowerBounds() const {
return true;
}
+mlir::Operation* traverseConverts(mlir::Operation *op) {
+ while (auto convert = llvm::dyn_cast_or_null<fir::ConvertOp>(op))
+ op = convert.getValue().getDefiningOp();
+ return op;
+}
+
+bool hlfir::Entity::mayBeOptional() const {
+ if (auto varIface = getIfVariableInterface())
+ return varIface.isOptional();
+ if (!isVariable())
+ return false;
+ // TODO: introduce a fir type to better identify optionals.
+ if (mlir::Operation* op = traverseConverts(getDefiningOp()))
+ return !llvm::isa<fir::AllocaOp, fir::AllocMemOp, fir::ReboxOp,
+ fir::EmboxOp>(op);
+ return true;
+}
+
fir::FortranVariableOpInterface
hlfir::genDeclare(mlir::Location loc, fir::FirOpBuilder &builder,
const fir::ExtendedValue &exv, llvm::StringRef name,
@@ -963,9 +981,68 @@ llvm::SmallVector<mlir::Value> hlfir::genLoopNestWithReductions(
return outerLoop->getResults();
}
+template <typename Lambda>
+static fir::ExtendedValue
+conditionnalyEvaluate(mlir::Location loc, fir::FirOpBuilder &builder,
+ mlir::Value condition, const Lambda &genIfTrue) {
+ mlir::OpBuilder::InsertPoint insertPt = builder.saveInsertionPoint();
+
+ // Evaluate in some region that will be moved into the actual ifOp (the actual
+ // ifOp can only be created when the result types are known).
+ auto badIfOp = builder.create<fir::IfOp>(loc, condition.getType(), condition,
+ /*withElseRegion=*/false);
+ mlir::Block *preparationBlock = &badIfOp.getThenRegion().front();
+ builder.setInsertionPointToStart(preparationBlock);
+ fir::ExtendedValue result = genIfTrue();
+ fir::ResultOp resultOp = result.match(
+ [&](const fir::CharBoxValue &box) -> fir::ResultOp {
+ return builder.create<fir::ResultOp>(
+ loc, mlir::ValueRange{box.getAddr(), box.getLen()});
+ },
+ [&](const mlir::Value &addr) -> fir::ResultOp {
+ return builder.create<fir::ResultOp>(loc, addr);
+ },
+ [&](const auto &) -> fir::ResultOp {
+ TODO(loc, "unboxing non scalar optional fir.box");
+ });
+ builder.restoreInsertionPoint(insertPt);
+
+ // Create actual fir.if operation.
+ auto ifOp =
+ builder.create<fir::IfOp>(loc, resultOp->getOperandTypes(), condition,
+ /*withElseRegion=*/true);
+ // Move evaluation into Then block,
+ preparationBlock->moveBefore(&ifOp.getThenRegion().back());
+ ifOp.getThenRegion().back().erase();
+ // Create absent result in the Else block.
+ builder.setInsertionPointToStart(&ifOp.getElseRegion().front());
+ llvm::SmallVector<mlir::Value> absentValues;
+ for (mlir::Type resTy : ifOp->getResultTypes()) {
+ if (fir::isa_ref_type(resTy) || fir::isa_box_type(resTy))
+ absentValues.emplace_back(builder.create<fir::AbsentOp>(loc, resTy));
+ else
+ absentValues.emplace_back(builder.create<fir::ZeroOp>(loc, resTy));
+ }
+ builder.create<fir::ResultOp>(loc, absentValues);
+ badIfOp->erase();
+
+ // Build fir::ExtendedValue from the result values.
+ builder.setInsertionPointAfter(ifOp);
+ return result.match(
+ [&](const fir::CharBoxValue &box) -> fir::ExtendedValue {
+ return fir::CharBoxValue{ifOp.getResult(0), ifOp.getResult(1)};
+ },
+ [&](const mlir::Value &) -> fir::ExtendedValue {
+ return ifOp.getResult(0);
+ },
+ [&](const auto &) -> fir::ExtendedValue {
+ TODO(loc, "unboxing non scalar optional fir.box");
+ });
+}
+
static fir::ExtendedValue translateVariableToExtendedValue(
mlir::Location loc, fir::FirOpBuilder &builder, hlfir::Entity variable,
- bool forceHlfirBase = false, bool contiguousHint = false) {
+ bool forceHlfirBase = false, bool contiguousHint = false, bool keepScalarOptionalBoxed = false) {
assert(variable.isVariable() && "must be a variable");
// When going towards FIR, use the original base value to avoid
// introducing descriptors at runtime when they are not required.
@@ -984,7 +1061,7 @@ static fir::ExtendedValue translateVariableToExtendedValue(
const bool contiguous = variable.isSimplyContiguous() || contiguousHint;
const bool isAssumedRank = variable.isAssumedRank();
if (!contiguous || variable.isPolymorphic() ||
- variable.isDerivedWithLengthParameters() || variable.isOptional() ||
+ variable.isDerivedWithLengthParameters() ||
isAssumedRank) {
llvm::SmallVector<mlir::Value> nonDefaultLbounds;
if (!isAssumedRank)
@@ -992,6 +1069,25 @@ static fir::ExtendedValue translateVariableToExtendedValue(
return fir::BoxValue(base, nonDefaultLbounds,
getExplicitTypeParams(variable));
}
+ if (variable.mayBeOptional()) {
+ if (!keepScalarOptionalBoxed && variable.isScalar()) {
+ mlir::Value isPresent = builder.create<fir::IsPresentOp>(
+ loc, builder.getI1Type(), variable);
+ return conditionnalyEvaluate(
+ loc, builder, isPresent, [&]() -> fir::ExtendedValue {
+ mlir::Value base = genVariableRawAddress(loc, builder, variable);
+ if (variable.isCharacter()) {
+ mlir::Value len =
+ genCharacterVariableLength(loc, builder, variable);
+ return fir::CharBoxValue{base, len};
+ }
+ return base;
+ });
+ }
+ llvm::SmallVector<mlir::Value> nonDefaultLbounds = getNonDefaultLowerBounds(loc, builder, variable);
+ return fir::BoxValue(base, nonDefaultLbounds,
+ getExplicitTypeParams(variable));
+ }
// Otherwise, the variable can be represented in a fir::ExtendedValue
// without the overhead of a fir.box.
base = genVariableRawAddress(loc, builder, variable);
@@ -1035,10 +1131,12 @@ hlfir::translateToExtendedValue(mlir::Location loc, fir::FirOpBuilder &builder,
std::pair<fir::ExtendedValue, std::optional<hlfir::CleanupFunction>>
hlfir::translateToExtendedValue(mlir::Location loc, fir::FirOpBuilder &builder,
- hlfir::Entity entity, bool contiguousHint) {
+ hlfir::Entity entity, bool contiguousHint,
+ bool keepScalarOptionalBoxed) {
if (entity.isVariable())
return {translateVariableToExtendedValue(loc, builder, entity, false,
- contiguousHint),
+ contiguousHint,
+ keepScalarOptionalBoxed),
std::nullopt};
if (entity.isProcedure()) {
diff --git a/flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIRIntrinsics.cpp b/flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIRIntrinsics.cpp
index bd12700f138386..7a8675a8a27a45 100644
--- a/flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIRIntrinsics.cpp
+++ b/flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIRIntrinsics.cpp
@@ -121,8 +121,14 @@ class HlfirIntrinsicConversion : public mlir::OpRewritePattern<OP> {
// simplified since the fir.box lowered here are now guarenteed to
// contain the local lower bounds thanks to the hlfir.declare (the extra
// rebox can be removed).
- auto [exv, cleanup] =
- hlfir::translateToExtendedValue(loc, builder, entity);
+ // When taking arguments as descriptors, the runtime expect absent
+ // OPTIONAL to be a nullptr to a descriptor, lowering has already
+ // prepared such descriptors // as needed, hence set
+ // keepScalarOptionalBoxed to avoid building descriptors with a null
+ // address for them.
+ auto [exv, cleanup] = hlfir::translateToExtendedValue(
+ loc, builder, entity, /*contiguous=*/false,
+ /*keepScalarOptionalBoxed=*/true);
if (cleanup)
cleanupFns.push_back(*cleanup);
ret.emplace_back(exv);
diff --git a/flang/test/HLFIR/assign-codegen.fir b/flang/test/HLFIR/assign-codegen.fir
index 5e48784284a8b4..7e03aa0bd464d8 100644
--- a/flang/test/HLFIR/assign-codegen.fir
+++ b/flang/test/HLFIR/assign-codegen.fir
@@ -429,11 +429,55 @@ func.func @test_upoly_expr_assignment(%arg0: !fir.class<!fir.array<?xnone>> {fir
// CHECK: }
func.func @test_scalar_box(%arg0: f32, %arg1: !fir.box<!fir.ptr<f32>>) {
- hlfir.assign %arg0 to %arg1 : f32, !fir.box<!fir.ptr<f32>>
+ %x = fir.declare %arg1 {uniq_name = "x"} : (!fir.box<!fir.ptr<f32>>) -> !fir.box<!fir.ptr<f32>>
+ hlfir.assign %arg0 to %x : f32, !fir.box<!fir.ptr<f32>>
return
}
// CHECK-LABEL: func.func @test_scalar_box(
// CHECK-SAME: %[[VAL_0:.*]]: f32,
// CHECK-SAME: %[[VAL_1:.*]]: !fir.box<!fir.ptr<f32>>) {
-// CHECK: %[[VAL_2:.*]] = fir.box_addr %[[VAL_1]] : (!fir.box<!fir.ptr<f32>>) -> !fir.ptr<f32>
-// CHECK: fir.store %[[VAL_0]] to %[[VAL_2]] : !fir.ptr<f32>
+// CHECK: %[[VAL_2:.*]] = fir.declare %[[VAL_1]] {uniq_name = "x"} : (!fir.box<!fir.ptr<f32>>) -> !fir.box<!fir.ptr<f32>>
+// CHECK: %[[VAL_3:.*]] = fir.box_addr %[[VAL_2]] : (!fir.box<!fir.ptr<f32>>) -> !fir.ptr<f32>
+// CHECK: fir.store %[[VAL_0]] to %[[VAL_3]] : !fir.ptr<f32>
+
+func.func @test_scalar_opt_box(%arg0: f32, %arg1: !fir.box<!fir.ptr<f32>>) {
+ %x = fir.declare %arg1 {fortran_attrs = #fir.var_attrs<optional>, uniq_name = "x"} : (!fir.box<!fir.ptr<f32>>) -> !fir.box<!fir.ptr<f32>>
+ hlfir.assign %arg0 to %x : f32, !fir.box<!fir.ptr<f32>>
+ return
+}
+// CHECK-LABEL: func.func @test_scalar_opt_box(
+// CHECK-SAME: %[[VAL_0:.*]]: f32,
+// CHECK-SAME: %[[VAL_1:.*]]: !fir.box<!fir.ptr<f32>>) {
+// 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>>
+// CHECK: %[[VAL_3:.*]] = fir.is_present %[[VAL_2]] : (!fir.box<!fir.ptr<f32>>) -> i1
+// CHECK: %[[VAL_4:.*]] = fir.if %[[VAL_3]] -> (!fir.ptr<f32>) {
+// CHECK: %[[VAL_5:.*]] = fir.box_addr %[[VAL_2]] : (!fir.box<!fir.ptr<f32>>) -> !fir.ptr<f32>
+// CHECK: fir.result %[[VAL_5]] : !fir.ptr<f32>
+// CHECK: } else {
+// CHECK: %[[VAL_6:.*]] = fir.absent !fir.ptr<f32>
+// CHECK: fir.result %[[VAL_6]] : !fir.ptr<f32>
+// CHECK: }
+// CHECK: fir.store %[[VAL_0]] to %[[VAL_4]] : !fir.ptr<f32>
+
+func.func @test_scalar_opt_char_box(%arg0: !fir.ref<!fir.char<1,10>>, %arg1: !fir.box<!fir.char<1,?>>) {
+ %x = fir.declare %arg1 {fortran_attrs = #fir.var_attrs<optional>, uniq_name = "x"} : (!fir.box<!fir.char<1,?>>) -> !fir.box<!fir.char<1,?>>
+ hlfir.assign %arg0 to %x : !fir.ref<!fir.char<1,10>>, !fir.box<!fir.char<1,?>>
+ return
+}
+// CHECK-LABEL: func.func @test_scalar_opt_char_box(
+// CHECK-SAME: %[[VAL_0:.*]]: !fir.ref<!fir.char<1,10>>,
+// CHECK-SAME: %[[VAL_1:.*]]: !fir.box<!fir.char<1,?>>) {
+// 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,?>>
+// CHECK: %[[VAL_3:.*]] = arith.constant 10 : index
+// CHECK: %[[VAL_4:.*]] = fir.is_present %[[VAL_2]] : (!fir.box<!fir.char<1,?>>) -> i1
+// CHECK: %[[VAL_5:.*]]:2 = fir.if %[[VAL_4]] -> (!fir.ref<!fir.char<1,?>>, index) {
+// CHECK: %[[VAL_6:.*]] = fir.box_addr %[[VAL_2]] : (!fir.box<!fir.char<1,?>>) -> !fir.ref<!fir.char<1,?>>
+// CHECK: %[[VAL_7:.*]] = fir.box_elesize %[[VAL_2]] : (!fir.box<!fir.char<1,?>>) -> index
+// CHECK: fir.result %[[VAL_6]], %[[VAL_7]] : !fir.ref<!fir.char<1,?>>, index
+// CHECK: } else {
+// CHECK: %[[VAL_8:.*]] = fir.absent !fir.ref<!fir.char<1,?>>
+// CHECK: %[[VAL_9:.*]] = fir.zero_bits index
+// CHECK: fir.result %[[VAL_8]], %[[VAL_9]] : !fir.ref<!fir.char<1,?>>, index
+// CHECK: }
+// ...
+// CHECK: fir.call @llvm.memmove.p0.p0.i64(
More information about the flang-commits
mailing list