[flang] fix isSimplyContiguous and isOptional hlfir::Entity methods #125215
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir Author: None (jeanPerier) ChangesSome 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 Hence:
Full diff: https://github.com/llvm/llvm-project/pull/125215.diff 4 Files Affected:
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(
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
github-actions bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jan 31, 2025
…lars to extented value" (#125222) Reverts llvm/llvm-project#125059 Broke OPTIONAL lowering for some intrinsics. I have a proper fix for review llvm/llvm-project#125215, but I would like to better test it, so I am reverting in the meantime.
|
FWIW this fixes https://github.com/llvm/llvm-test-suite/blob/main/Fortran/gfortran/regression/optional_absent_5.f90 and https://github.com/llvm/llvm-test-suite/blob/main/Fortran/gfortran/regression/optional_absent_4.f90 on my system (AArch64 Linux). If you need any assistance with running the other tests/test suites, let me know. |
tblah
approved these changes
Jan 31, 2025
Comment on lines
+237
to
+238
| return !llvm::isa<fir::AllocaOp, fir::AllocMemOp, fir::ReboxOp, | ||
| fir::EmboxOp, fir::LoadOp>(op); |
There was a problem hiding this comment.
nit: I'm nervous about lists like this because it is easy to forget to update them. Maybe check if it is a function argument?
There was a problem hiding this comment.
Me, too, hence the comment about representing optional in the FIR type system.
Maybe check if it is a function argument?
This would work for user OPTIONAL (assuming one is always able to walk back to the argument, which is a broad assumption), but not "lowering generated" optionals that are generated when passing deallocated pointers/allocatable. Hence it is safer to list what is known fir.box that are know to not be OPTIONAL because of their producers.
From a correctness point of view, missing elements in the list does not matter, it will just trigger uglier code to be generated (the fir.if), and given scalar fir.box are pretty rare in lowering, it will not occur a lot.
There was a problem hiding this comment.
Agreed. Thanks for explaining.
They also reverted the original (hence the current conflict I think), so we are back to green for now. |
…lue (llvm#125059) The code in `translateToExtendedValue(hlfir::Entity)` was not getting rid of the fir.box for scalars because isSimplyContiguous() returned false for them. This created issues downstream because utilities using fir::ExtendedValue were not implemented to work with intrinsic scalars fir.box. fir.box of intrinsic scalars are not very commonly used as hlfir::Entity but they are allowed and should work where accepted.
jeanPerier
changed the title
Co-authored-by: Slava Zakharin <szakharin@nvidia.com>
jeanPerier
commented
Feb 3, 2025
Icohedron
pushed a commit
to Icohedron/llvm-project
that referenced
this pull request
Feb 11, 2025
…lvm#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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix isSimplyContiguous:
translateToExtendedValueFix isOptional:
It is not reliable when the memory SSA value cannot be linked to a declare. This is exposed by the
isSimplyContiguousfix,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).
Hence:
isOptionalinto a safermayBeOptionaltranslateToExtendedValueusage 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.