[flang] Do not move finalized function results in lowering

[jeanPerier]

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.

[llvmbot]

@llvm/pr-subscribers-flang-fir-hlfir

Author: None (jeanPerier)

Changes

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 --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

[vzakhari]

LGTM. Thank you!

[clementval]

LGTM!

[vzakhari]

Do you think https://reviews.llvm.org/D156187 can now be reverted? Or do we still need to keep it so that the variable has default lbounds when the finalization function is run?

[jeanPerier]

Do you think https://reviews.llvm.org/D156187 can now be reverted? Or do we still need to keep it so that the variable has default lbounds when the finalization function is run?

I think we should be able to revert it yes, thanks for pointing to it.

The hlfir.declare/hlfir.as_expr have the same effect (except that they also convey that statically in the IR on the caller side). I do not think it is possible to observe the allocatable lower bounds in the finalization because C786 says that the argument of final subroutine "shall be a [...] nonpointer, nonallocatable", which implies that the lower bounds in finalization will always be ones (or whatever the final subroutine locally defined in its specification expressions), regardless of what is being finalized.

I will test in another patch.