[flang-commits] [flang] [flang] Do not move finalized function results in lowering (PR #80683)
via flang-commits
flang-commits at lists.llvm.org
Mon Feb 5 06:13:25 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-flang-fir-hlfir
Author: None (jeanPerier)
<details>
<summary>Changes</summary>
Fortran requires finalizing function results when the result type have final procedures.
Lowering was unconditionally "moving" function results into values "hlfir.expr". This is not correct when the results are finalized because it means the function result storage will be used after the hlfir.expr.
Only move function results that are not finalized.
---
Full diff: https://github.com/llvm/llvm-project/pull/80683.diff
4 Files Affected:
- (modified) flang/include/flang/Lower/ConvertCall.h (+3-1)
- (modified) flang/lib/Lower/ConvertCall.cpp (+26-14)
- (modified) flang/lib/Lower/ConvertExpr.cpp (+8-4)
- (modified) flang/test/Lower/HLFIR/function-return-as-expr.f90 (+4-3)
``````````diff
diff --git a/flang/include/flang/Lower/ConvertCall.h b/flang/include/flang/Lower/ConvertCall.h
index f8171236bb39d..bc082907e6176 100644
--- a/flang/include/flang/Lower/ConvertCall.h
+++ b/flang/include/flang/Lower/ConvertCall.h
@@ -30,7 +30,9 @@ namespace Fortran::lower {
/// link to internal procedures.
/// \p isElemental must be set to true if elemental call is being produced.
/// It is only used for HLFIR.
-fir::ExtendedValue genCallOpAndResult(
+/// The returned boolean indicates if finalization has been emitted in
+/// \p stmtCtx for the result.
+std::pair<fir::ExtendedValue, bool> genCallOpAndResult(
mlir::Location loc, Fortran::lower::AbstractConverter &converter,
Fortran::lower::SymMap &symMap, Fortran::lower::StatementContext &stmtCtx,
Fortran::lower::CallerInterface &caller, mlir::FunctionType callSiteType,
diff --git a/flang/lib/Lower/ConvertCall.cpp b/flang/lib/Lower/ConvertCall.cpp
index bb8fd2e945f43..b10a51229a082 100644
--- a/flang/lib/Lower/ConvertCall.cpp
+++ b/flang/lib/Lower/ConvertCall.cpp
@@ -147,7 +147,7 @@ static bool mustCastFuncOpToCopeWithImplicitInterfaceMismatch(
return false;
}
-fir::ExtendedValue Fortran::lower::genCallOpAndResult(
+std::pair<fir::ExtendedValue, bool> Fortran::lower::genCallOpAndResult(
mlir::Location loc, Fortran::lower::AbstractConverter &converter,
Fortran::lower::SymMap &symMap, Fortran::lower::StatementContext &stmtCtx,
Fortran::lower::CallerInterface &caller, mlir::FunctionType callSiteType,
@@ -478,6 +478,7 @@ fir::ExtendedValue Fortran::lower::genCallOpAndResult(
[](const auto &) {});
// 7.5.6.3 point 5. Derived-type finalization for nonpointer function.
+ bool resultIsFinalized = false;
// Check if the derived-type is finalizable if it is a monomorphic
// derived-type.
// For polymorphic and unlimited polymorphic enities call the runtime
@@ -499,6 +500,7 @@ fir::ExtendedValue Fortran::lower::genCallOpAndResult(
fir::runtime::genDerivedTypeDestroy(*bldr, loc,
fir::getBase(*allocatedResult));
});
+ resultIsFinalized = true;
} else {
const Fortran::semantics::DerivedTypeSpec &typeSpec =
retTy->GetDerivedTypeSpec();
@@ -513,14 +515,17 @@ fir::ExtendedValue Fortran::lower::genCallOpAndResult(
mlir::Value box = bldr->createBox(loc, *allocatedResult);
fir::runtime::genDerivedTypeDestroy(*bldr, loc, box);
});
+ resultIsFinalized = true;
}
}
}
- return *allocatedResult;
+ return {*allocatedResult, resultIsFinalized};
}
+ // subroutine call
if (!resultType)
- return mlir::Value{}; // subroutine call
+ return {fir::ExtendedValue{mlir::Value{}}, /*resultIsFinalized=*/false};
+
// For now, Fortran return values are implemented with a single MLIR
// function return value.
assert(callNumResults == 1 && "Expected exactly one result in FUNCTION call");
@@ -533,10 +538,10 @@ fir::ExtendedValue Fortran::lower::genCallOpAndResult(
funcType.getResults()[0].dyn_cast<fir::CharacterType>();
mlir::Value len = builder.createIntegerConstant(
loc, builder.getCharacterLengthType(), charTy.getLen());
- return fir::CharBoxValue{callResult, len};
+ return {fir::CharBoxValue{callResult, len}, /*resultIsFinalized=*/false};
}
- return callResult;
+ return {callResult, /*resultIsFinalized=*/false};
}
static hlfir::EntityWithAttributes genStmtFunctionRef(
@@ -1389,7 +1394,7 @@ genUserCall(Fortran::lower::PreparedActualArguments &loweredActuals,
// Prepare lowered arguments according to the interface
// and map the lowered values to the dummy
// arguments.
- fir::ExtendedValue result = Fortran::lower::genCallOpAndResult(
+ auto [result, resultIsFinalized] = Fortran::lower::genCallOpAndResult(
loc, callContext.converter, callContext.symMap, callContext.stmtCtx,
caller, callSiteType, callContext.resultType,
callContext.isElementalProcWithArrayArgs());
@@ -1409,15 +1414,22 @@ genUserCall(Fortran::lower::PreparedActualArguments &loweredActuals,
if (!fir::isPointerType(fir::getBase(result).getType())) {
resultEntity = loadTrivialScalar(loc, builder, resultEntity);
-
if (resultEntity.isVariable()) {
- // Function result must not be freed, since it is allocated on the stack.
- // Note that in non-elemental case, genCallOpAndResult()
- // is responsible for establishing the clean-up that destroys
- // the derived type result or deallocates its components
- // without finalization.
- auto asExpr = builder.create<hlfir::AsExprOp>(
- loc, resultEntity, /*mustFree=*/builder.createBool(loc, false));
+ // If the result has no finalization, it can be moved into an expression.
+ // In such case, the expression should not be freed after its use since
+ // the result is stack allocated or deallocation (for allocatable results)
+ // was already inserted in genCallOpAndResult.
+ // If the result has finalization, it cannot be moved because use of its
+ // value have been created in the statement context and may be emitted
+ // after the hlfir.expr destroy.
+ // TODO: find a way to move the finalization on the hlfir.expr to avoid a
+ // copy. This is likely better done in a pass since it is not clear here
+ // when an where the hlfir.destroy or hlfir.end_associate will be emitted
+ // for the expression.
+ mlir::Value mustFree =
+ resultIsFinalized ? mlir::Value{} : builder.createBool(loc, false);
+ auto asExpr = builder.create<hlfir::AsExprOp>(loc, resultEntity,
+ /*mustFree=*/mustFree);
resultEntity = hlfir::EntityWithAttributes{asExpr.getResult()};
}
}
diff --git a/flang/lib/Lower/ConvertExpr.cpp b/flang/lib/Lower/ConvertExpr.cpp
index a2b28aa2e0491..d157db2cde496 100644
--- a/flang/lib/Lower/ConvertExpr.cpp
+++ b/flang/lib/Lower/ConvertExpr.cpp
@@ -2846,8 +2846,10 @@ class ScalarExprLowering {
}
}
- ExtValue result = Fortran::lower::genCallOpAndResult(
- loc, converter, symMap, stmtCtx, caller, callSiteType, resultType);
+ ExtValue result =
+ Fortran::lower::genCallOpAndResult(loc, converter, symMap, stmtCtx,
+ caller, callSiteType, resultType)
+ .first;
// Sync pointers and allocatables that may have been modified during the
// call.
@@ -4866,8 +4868,10 @@ class ArrayExprLowering {
[&](const auto &) { return fir::getBase(exv); });
caller.placeInput(argIface, arg);
}
- return Fortran::lower::genCallOpAndResult(
- loc, converter, symMap, getElementCtx(), caller, callSiteType, retTy);
+ return Fortran::lower::genCallOpAndResult(loc, converter, symMap,
+ getElementCtx(), caller,
+ callSiteType, retTy)
+ .first;
};
}
diff --git a/flang/test/Lower/HLFIR/function-return-as-expr.f90 b/flang/test/Lower/HLFIR/function-return-as-expr.f90
index 7c8e73779d79e..dadb9f7c96478 100644
--- a/flang/test/Lower/HLFIR/function-return-as-expr.f90
+++ b/flang/test/Lower/HLFIR/function-return-as-expr.f90
@@ -66,11 +66,12 @@ function inner()
end function inner
end subroutine test4
! CHECK-LABEL: func.func @_QPtest4() {
-! CHECK: %[[VAL_6:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = ".tmp.func_result"} : (!fir.ref<!fir.class<!fir.heap<none>>>) -> (!fir.ref<!fir.class<!fir.heap<none>>>, !fir.ref<!fir.class<!fir.heap<none>>>)
+! CHECK: %[[VAL_6:.*]]:2 = hlfir.declare %[[VAL_0:.*]] {uniq_name = ".tmp.func_result"} : (!fir.ref<!fir.class<!fir.heap<none>>>) -> (!fir.ref<!fir.class<!fir.heap<none>>>, !fir.ref<!fir.class<!fir.heap<none>>>)
! CHECK: %[[VAL_7:.*]] = fir.load %[[VAL_6]]#0 : !fir.ref<!fir.class<!fir.heap<none>>>
-! CHECK: %[[VAL_8:.*]] = arith.constant false
-! CHECK: %[[VAL_9:.*]] = hlfir.as_expr %[[VAL_7]] move %[[VAL_8]] : (!fir.class<!fir.heap<none>>, i1) -> !hlfir.expr<none?>
+! CHECK: %[[VAL_9:.*]] = hlfir.as_expr %[[VAL_7]] : (!fir.class<!fir.heap<none>>) -> !hlfir.expr<none?>
! CHECK: hlfir.assign %[[VAL_9]] to %{{.*}}#0 realloc : !hlfir.expr<none?>, !fir.ref<!fir.class<!fir.heap<none>>>
+! CHECK: %[[VAL_10:.*]] = fir.convert %[[VAL_0]] : (!fir.ref<!fir.class<!fir.heap<none>>>) -> !fir.box<none>
+! CHECK: fir.call @_FortranADestroy(%[[VAL_10]]) fastmath<contract> : (!fir.box<none>) -> none
subroutine test5
use types
``````````
</details>
https://github.com/llvm/llvm-project/pull/80683
More information about the flang-commits
mailing list