Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flang][hlfir] use fir.type_info to skip runtime call if nofinal is set #68397

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

jeanPerier
Copy link
Contributor

HLFIR was always calling Destroy runtime when doing derived type scalar assignments because the IR did not contain the info of whether finalization was needed or not.

This info is now available in fir.type_info which allow skipping the runtime call when not needed.

Also, when finalization is needed, simply use Assign runtime. This makes no difference from a semantic point of view with the current code that generated a call to Destroy and did the assignment inline, but if some piece of runtime must be called anyway, it is simpler to just call Assign that deals with everything.

HLFIR was always calling Destroy runtime when doing derived type scalar
assignments because the IR did not contain the info of whether
finalization was needed or not.

This info is now available in fir.type_info which allow skipping the
runtime call when not needed.

Also, when finalization is needed, simply use Assign runtime. This makes
no difference from a semantic point of view with the
current code that generated a call to Destroy and did the assignment
inline, but if some piece of runtime must be called anyway,
it is simpler to just call Assign that deals with everything.
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Oct 6, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 6, 2023

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

Changes

HLFIR was always calling Destroy runtime when doing derived type scalar assignments because the IR did not contain the info of whether finalization was needed or not.

This info is now available in fir.type_info which allow skipping the runtime call when not needed.

Also, when finalization is needed, simply use Assign runtime. This makes no difference from a semantic point of view with the current code that generated a call to Destroy and did the assignment inline, but if some piece of runtime must be called anyway, it is simpler to just call Assign that deals with everything.


Full diff: https://github.com/llvm/llvm-project/pull/68397.diff

4 Files Affected:

  • (modified) flang/lib/Optimizer/Builder/FIRBuilder.cpp (+11-7)
  • (added) flang/test/HLFIR/assign-codegen-derived.fir (+29)
  • (modified) flang/test/HLFIR/assign-codegen.fir (+10-9)
  • (modified) flang/test/Lower/derived-type-finalization.f90 (+6-4)
diff --git a/flang/lib/Optimizer/Builder/FIRBuilder.cpp b/flang/lib/Optimizer/Builder/FIRBuilder.cpp
index a14f3106a723291..e9d18a054e35af8 100644
--- a/flang/lib/Optimizer/Builder/FIRBuilder.cpp
+++ b/flang/lib/Optimizer/Builder/FIRBuilder.cpp
@@ -1261,6 +1261,15 @@ static bool recordTypeCanBeMemCopied(fir::RecordType recordType) {
   return true;
 }
 
+static bool mayHaveFinalizer(fir::RecordType recordType,
+                             fir::FirOpBuilder &builder) {
+  if (auto typeInfo = builder.getModule().lookupSymbol<fir::TypeInfoOp>(
+          recordType.getName()))
+    return !typeInfo.getNoFinal();
+  // No info, be pessimistic.
+  return true;
+}
+
 void fir::factory::genRecordAssignment(fir::FirOpBuilder &builder,
                                        mlir::Location loc,
                                        const fir::ExtendedValue &lhs,
@@ -1277,7 +1286,8 @@ void fir::factory::genRecordAssignment(fir::FirOpBuilder &builder,
                         fir::getBase(rhs).getType().isa<fir::BaseBoxType>();
   auto recTy = baseTy.dyn_cast<fir::RecordType>();
   assert(recTy && "must be a record type");
-  if (hasBoxOperands || !recordTypeCanBeMemCopied(recTy)) {
+  if ((needFinalization && mayHaveFinalizer(recTy, builder)) ||
+      hasBoxOperands || !recordTypeCanBeMemCopied(recTy)) {
     auto to = fir::getBase(builder.createBox(loc, lhs));
     auto from = fir::getBase(builder.createBox(loc, rhs));
     // The runtime entry point may modify the LHS descriptor if it is
@@ -1294,12 +1304,6 @@ void fir::factory::genRecordAssignment(fir::FirOpBuilder &builder,
     return;
   }
 
-  // Finalize LHS on intrinsic assignment.
-  if (needFinalization) {
-    mlir::Value box = builder.createBox(loc, lhs);
-    fir::runtime::genDerivedTypeDestroy(builder, loc, box);
-  }
-
   // Otherwise, the derived type has compile time constant size and for which
   // the component by component assignment can be replaced by a memory copy.
   // Since we do not know the size of the derived type in lowering, do a
diff --git a/flang/test/HLFIR/assign-codegen-derived.fir b/flang/test/HLFIR/assign-codegen-derived.fir
new file mode 100644
index 000000000000000..c45c118ed46c594
--- /dev/null
+++ b/flang/test/HLFIR/assign-codegen-derived.fir
@@ -0,0 +1,29 @@
+// Test hlfir.assign code generation to FIR of derived type requiring
+// or not finalization.
+
+// RUN: fir-opt %s -convert-hlfir-to-fir | FileCheck %s
+
+!t_simple = !fir.type<simple{i:i32}>
+fir.type_info @simple noinit nodestroy nofinal : !t_simple
+
+func.func @test_simple(%a: !fir.ref<!t_simple>, %b: !fir.ref<!t_simple>) {
+  hlfir.assign %b to %a : !fir.ref<!t_simple>, !fir.ref<!t_simple>
+  return
+}
+// CHECK-LABEL: func.func @test_simple(
+// CHECK-NOT: Destroy
+// CHECK:  %[[VAL_1:.*]] = fir.coordinate_of %{{.*}}, %{{.*}} : (!fir.ref<!fir.type<simple{i:i32}>>, !fir.field) -> !fir.ref<i32>
+// CHECK:  %[[VAL_3:.*]] = fir.coordinate_of %{{.*}}, %{{.*}} : (!fir.ref<!fir.type<simple{i:i32}>>, !fir.field) -> !fir.ref<i32>
+// CHECK:  %[[VAL_4:.*]] = fir.load %[[VAL_1]] : !fir.ref<i32>
+// CHECK:  fir.store %[[VAL_4]] to %[[VAL_3]] : !fir.ref<i32>
+
+
+!t_with_final = !fir.type<with_final{i:i32}>
+fir.type_info @with_final noinit : !t_with_final
+
+func.func @test_with_final(%a: !fir.ref<!t_with_final>, %b: !fir.ref<!t_with_final>) {
+  hlfir.assign %b to %a : !fir.ref<!t_with_final>, !fir.ref<!t_with_final>
+  return
+}
+// CHECK-LABEL: func.func @test_with_final(
+// CHECK:  fir.call @_FortranAAssign
diff --git a/flang/test/HLFIR/assign-codegen.fir b/flang/test/HLFIR/assign-codegen.fir
index 5474ef5393df412..2211676eac58015 100644
--- a/flang/test/HLFIR/assign-codegen.fir
+++ b/flang/test/HLFIR/assign-codegen.fir
@@ -347,21 +347,21 @@ func.func @_QFPtest_scalar_lhs_finalization(%arg0: !fir.ref<!fir.type<_QMa8vTt1{
 // CHECK-LABEL:   func.func @_QFPtest_scalar_lhs_finalization(
 // CHECK-SAME:                                                %[[VAL_0:.*]]: !fir.ref<!fir.type<_QMa8vTt1{val:i32}>> {fir.bindc_name = "s1"},
 // CHECK-SAME:                                                %[[VAL_1:.*]]: !fir.ref<!fir.type<_QMa8vTt1{val:i32}>> {fir.bindc_name = "s2"}) {
+// CHECK:           %[[BOX:.*]] = fir.alloca !fir.box<!fir.type<_QMa8vTt1{val:i32}>>
 // CHECK:           %[[VAL_2:.*]] = fir.declare %[[VAL_0]] {uniq_name = "_QFFtest_scalar_lhs_finalizationEs1"} : (!fir.ref<!fir.type<_QMa8vTt1{val:i32}>>) -> !fir.ref<!fir.type<_QMa8vTt1{val:i32}>>
 // CHECK:           %[[VAL_3:.*]] = fir.declare %[[VAL_1]] {uniq_name = "_QFFtest_scalar_lhs_finalizationEs2"} : (!fir.ref<!fir.type<_QMa8vTt1{val:i32}>>) -> !fir.ref<!fir.type<_QMa8vTt1{val:i32}>>
+
 // CHECK:           %[[VAL_4:.*]] = fir.embox %[[VAL_2]] : (!fir.ref<!fir.type<_QMa8vTt1{val:i32}>>) -> !fir.box<!fir.type<_QMa8vTt1{val:i32}>>
-// CHECK:           %[[VAL_5:.*]] = fir.convert %[[VAL_4]] : (!fir.box<!fir.type<_QMa8vTt1{val:i32}>>) -> !fir.box<none>
-// CHECK:           %[[VAL_6:.*]] = fir.call @_FortranADestroy(%[[VAL_5]]) : (!fir.box<none>) -> none
-// CHECK:           %[[VAL_7:.*]] = fir.field_index val, !fir.type<_QMa8vTt1{val:i32}>
-// CHECK:           %[[VAL_8:.*]] = fir.coordinate_of %[[VAL_3]], %[[VAL_7]] : (!fir.ref<!fir.type<_QMa8vTt1{val:i32}>>, !fir.field) -> !fir.ref<i32>
-// CHECK:           %[[VAL_9:.*]] = fir.field_index val, !fir.type<_QMa8vTt1{val:i32}>
-// CHECK:           %[[VAL_10:.*]] = fir.coordinate_of %[[VAL_2]], %[[VAL_9]] : (!fir.ref<!fir.type<_QMa8vTt1{val:i32}>>, !fir.field) -> !fir.ref<i32>
-// CHECK:           %[[VAL_11:.*]] = fir.load %[[VAL_8]] : !fir.ref<i32>
-// CHECK:           fir.store %[[VAL_11]] to %[[VAL_10]] : !fir.ref<i32>
+// CHECK:           %[[VAL_5:.*]] = fir.embox %[[VAL_3]] : (!fir.ref<!fir.type<_QMa8vTt1{val:i32}>>) -> !fir.box<!fir.type<_QMa8vTt1{val:i32}>>
+// CHECK:           fir.store %[[VAL_4]] to %[[BOX]] : !fir.ref<!fir.box<!fir.type<_QMa8vTt1{val:i32}>>>
+// CHECK:           %[[VAL_10:.*]] = fir.convert %[[BOX]] : (!fir.ref<!fir.box<!fir.type<_QMa8vTt1{val:i32}>>>) -> !fir.ref<!fir.box<none>>
+// CHECK:           %[[VAL_11:.*]] = fir.convert %[[VAL_5]] : (!fir.box<!fir.type<_QMa8vTt1{val:i32}>>) -> !fir.box<none>
+// CHECK:           %[[VAL_12:.*]] = fir.convert %{{.*}} : (!fir.ref<!fir.char<1,{{.*}}>>) -> !fir.ref<i8>
+// CHECK:           %[[VAL_13:.*]] = fir.call @_FortranAAssign(%[[VAL_10]], %[[VAL_11]], %[[VAL_12]], %{{.*}}) : (!fir.ref<!fir.box<none>>, !fir.box<none>, !fir.ref<i8>, i32) -> none
 // CHECK:           return
 // CHECK:         }
 
-// Check that Destroy() is not called for temporary LHS.
+// Check that Assign() or Destroy() is not called for temporary LHS.
 func.func @_QFPtest_scalar_temp_lhs_no_finalization(%arg0: !fir.ref<!fir.type<_QMa8vTt1{val:i32}>> {fir.bindc_name = "s1"}, %arg1: !fir.ref<!fir.type<_QMa8vTt1{val:i32}>> {fir.bindc_name = "s2"}) {
   %0:2 = hlfir.declare %arg0 {uniq_name = "_QFFtest_scalar_lhs_finalizationEs1"} : (!fir.ref<!fir.type<_QMa8vTt1{val:i32}>>) -> (!fir.ref<!fir.type<_QMa8vTt1{val:i32}>>, !fir.ref<!fir.type<_QMa8vTt1{val:i32}>>)
   %1:2 = hlfir.declare %arg1 {uniq_name = "_QFFtest_scalar_lhs_finalizationEs2"} : (!fir.ref<!fir.type<_QMa8vTt1{val:i32}>>) -> (!fir.ref<!fir.type<_QMa8vTt1{val:i32}>>, !fir.ref<!fir.type<_QMa8vTt1{val:i32}>>)
@@ -369,6 +369,7 @@ func.func @_QFPtest_scalar_temp_lhs_no_finalization(%arg0: !fir.ref<!fir.type<_Q
   return
 }
 // CHECK-LABEL:   func.func @_QFPtest_scalar_temp_lhs_no_finalization(
+// CHECK-NOT: fir.call @_FortranAAssign
 // CHECK-NOT: fir.call @_FortranADestroy
 
 func.func @test_upoly_expr_assignment(%arg0: !fir.class<!fir.array<?xnone>> {fir.bindc_name = "y"}) {
diff --git a/flang/test/Lower/derived-type-finalization.f90 b/flang/test/Lower/derived-type-finalization.f90
index d0fbfe8906c4d1d..86e2e274e9d7e5c 100644
--- a/flang/test/Lower/derived-type-finalization.f90
+++ b/flang/test/Lower/derived-type-finalization.f90
@@ -55,11 +55,12 @@ subroutine test_lhs_allocatable()
   end subroutine
 
 ! CHECK-LABEL: func.func @_QMderived_type_finalizationPtest_lhs() {
+! CHECK: %[[BOXREF:.*]] = fir.alloca !fir.box<!fir.type<_QMderived_type_finalizationTt1{a:i32}>>
 ! CHECK: %[[LHS:.*]] = fir.alloca !fir.type<_QMderived_type_finalizationTt1{a:i32}> {bindc_name = "lhs", uniq_name = "_QMderived_type_finalizationFtest_lhsElhs"}
-! CHECK: %[[RHS:.*]] = fir.alloca !fir.type<_QMderived_type_finalizationTt1{a:i32}> {bindc_name = "rhs", uniq_name = "_QMderived_type_finalizationFtest_lhsErhs"}
 ! CHECK: %[[EMBOX:.*]] = fir.embox %[[LHS]] : (!fir.ref<!fir.type<_QMderived_type_finalizationTt1{a:i32}>>) -> !fir.box<!fir.type<_QMderived_type_finalizationTt1{a:i32}>>
-! CHECK: %[[BOX_NONE:.*]] = fir.convert %[[EMBOX]] : (!fir.box<!fir.type<_QMderived_type_finalizationTt1{a:i32}>>) -> !fir.box<none>
-! CHECK: %{{.*}} = fir.call @_FortranADestroy(%[[BOX_NONE]]) {{.*}} : (!fir.box<none>) -> none
+! CHECK: fir.store %[[EMBOX]] to %[[BOXREF]] : !fir.ref<!fir.box<!fir.type<_QMderived_type_finalizationTt1{a:i32}>>>
+! CHECK: %[[BOX_NONE:.*]] = fir.convert %[[BOXREF]] : (!fir.ref<!fir.box<!fir.type<_QMderived_type_finalizationTt1{a:i32}>>>) -> !fir.ref<!fir.box<none>>
+! CHECK: %{{.*}} = fir.call @_FortranAAssign(%[[BOX_NONE]], {{.*}}
 
 ! CHECK-LABEL: func.func @_QMderived_type_finalizationPtest_lhs_allocatable() {
 ! CHECK: %[[LHS:.*]] = fir.alloca !fir.box<!fir.heap<!fir.type<_QMderived_type_finalizationTt1{a:i32}>>> {bindc_name = "lhs", uniq_name = "_QMderived_type_finalizationFtest_lhs_allocatableElhs"}
@@ -210,7 +211,8 @@ function no_func_ret_finalize() result(ty)
   end function
 
 ! CHECK-LABEL: func.func @_QMderived_type_finalizationPno_func_ret_finalize() -> !fir.type<_QMderived_type_finalizationTt1{a:i32}> {
-! CHECK: %{{.*}} = fir.call @_FortranADestroy
+! CHECK: %{{.*}} = fir.call @_FortranAAssign
+! CHECK-NOT: fir.call @_FortranADestroy
 ! CHECK: return %{{.*}} : !fir.type<_QMderived_type_finalizationTt1{a:i32}>
 
   function copy(a) result(ty)

Copy link
Contributor

@tblah tblah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

Copy link
Contributor

@vzakhari vzakhari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@clementval clementval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jeanPerier jeanPerier merged commit 87b2682 into llvm:main Oct 9, 2023
4 checks passed
@jeanPerier jeanPerier deleted the jpr-assign-instead-of-destroy-3 branch October 9, 2023 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:fir-hlfir flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants