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] Fixed cleanup code placement indeterminism in OrderedAssignments. #66811

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

vzakhari
Copy link
Contributor

I had to remove test3() case in 73086da
to fix the buildbots. This patch brings it back with proper fix.

…ssignments.

I had to remove test3() case in 73086da
to fix the buildbots. This patch brings it back with proper fix.
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Sep 19, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 19, 2023

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

Changes

I had to remove test3() case in 73086da
to fix the buildbots. This patch brings it back with proper fix.


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

2 Files Affected:

  • (modified) flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIROrderedAssignments.cpp (+6-2)
  • (modified) flang/test/HLFIR/order_assignments/user-defined-assignment-finalization.fir (+123)
diff --git a/flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIROrderedAssignments.cpp b/flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIROrderedAssignments.cpp
index 66596f58f9fea92..c4aee7d39e4a3c5 100644
--- a/flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIROrderedAssignments.cpp
+++ b/flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIROrderedAssignments.cpp
@@ -243,7 +243,8 @@ class OrderedAssignmentRewriter {
   template <typename T>
   fir::factory::TemporaryStorage *insertSavedEntity(mlir::Region &region,
                                                     T &&temp) {
-    auto inserted = savedEntities.try_emplace(&region, std::forward<T>(temp));
+    auto inserted =
+        savedEntities.insert(std::make_pair(&region, std::forward<T>(temp)));
     assert(inserted.second && "temp must have been emplaced");
     return &inserted.first->second;
   }
@@ -267,7 +268,10 @@ class OrderedAssignmentRewriter {
 
   /// Map of temporary storage to keep track of saved entity once the run
   /// that saves them has been lowered. It is kept in-between runs.
-  llvm::DenseMap<mlir::Region *, fir::factory::TemporaryStorage> savedEntities;
+  /// llvm::MapVector is used to guarantee deterministic order
+  /// of iterating through savedEntities (e.g. for generating
+  /// destruction code for the temporary storages).
+  llvm::MapVector<mlir::Region *, fir::factory::TemporaryStorage> savedEntities;
   /// Map holding the values that were saved in the current run and that also
   /// need to be used (because their construct will be visited). It is reset
   /// after each run. It avoids having to store and fetch in the temporary
diff --git a/flang/test/HLFIR/order_assignments/user-defined-assignment-finalization.fir b/flang/test/HLFIR/order_assignments/user-defined-assignment-finalization.fir
index 643ade4afeb2825..bbb589169a80a2d 100644
--- a/flang/test/HLFIR/order_assignments/user-defined-assignment-finalization.fir
+++ b/flang/test/HLFIR/order_assignments/user-defined-assignment-finalization.fir
@@ -158,3 +158,126 @@ func.func @_QPtest2() {
 // CHECK:           fir.call @llvm.stackrestore.p0(%[[VAL_5]]) fastmath<contract> : (!fir.ref<i8>) -> ()
 // CHECK:           return
 // CHECK:         }
+
+//! subroutine test3(y)
+//!   use types
+//!   interface
+//!      function new_obja()
+//!        use types
+//!        type(ud_assign) :: new_obja(2)
+//!      end function new_obja
+//!   end interface
+//!   integer :: y(2)
+//!   type(ud_assign), save :: x(2)
+//!   where (y == 1) x = new_obja()
+//! end subroutine test3
+func.func @_QPtest3(%arg0: !fir.ref<!fir.array<2xi32>> {fir.bindc_name = "y"}) {
+  %c1_i32 = arith.constant 1 : i32
+  %c2 = arith.constant 2 : index
+  %0 = fir.alloca !fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>> {bindc_name = ".result"}
+  %1 = fir.address_of(@_QFtest3Ex) : !fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>
+  %2 = fir.shape %c2 : (index) -> !fir.shape<1>
+  %3:2 = hlfir.declare %1(%2) {uniq_name = "_QFtest3Ex"} : (!fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>, !fir.shape<1>) -> (!fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>, !fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>)
+  %4:2 = hlfir.declare %arg0(%2) {uniq_name = "_QFtest3Ey"} : (!fir.ref<!fir.array<2xi32>>, !fir.shape<1>) -> (!fir.ref<!fir.array<2xi32>>, !fir.ref<!fir.array<2xi32>>)
+  hlfir.where {
+    %5 = hlfir.elemental %2 unordered : (!fir.shape<1>) -> !hlfir.expr<2x!fir.logical<4>> {
+    ^bb0(%arg1: index):
+      %6 = hlfir.designate %4#0 (%arg1)  : (!fir.ref<!fir.array<2xi32>>, index) -> !fir.ref<i32>
+      %7 = fir.load %6 : !fir.ref<i32>
+      %8 = arith.cmpi eq, %7, %c1_i32 : i32
+      %9 = fir.convert %8 : (i1) -> !fir.logical<4>
+      hlfir.yield_element %9 : !fir.logical<4>
+    }
+    hlfir.yield %5 : !hlfir.expr<2x!fir.logical<4>> cleanup {
+      hlfir.destroy %5 : !hlfir.expr<2x!fir.logical<4>>
+    }
+  } do {
+    hlfir.region_assign {
+      %5 = fir.call @llvm.stacksave.p0() fastmath<contract> : () -> !fir.ref<i8>
+      %6 = fir.call @_QPnew_obja() fastmath<contract> : () -> !fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>
+      fir.save_result %6 to %0(%2) : !fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>, !fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>, !fir.shape<1>
+      %7:2 = hlfir.declare %0(%2) {uniq_name = ".tmp.func_result"} : (!fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>, !fir.shape<1>) -> (!fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>, !fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>)
+      hlfir.yield %7#0 : !fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>> cleanup {
+        %8 = fir.embox %0(%2) : (!fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>, !fir.shape<1>) -> !fir.box<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>
+        %9 = fir.convert %8 : (!fir.box<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>) -> !fir.box<none>
+        %10 = fir.call @_FortranADestroy(%9) fastmath<contract> : (!fir.box<none>) -> none
+        fir.call @llvm.stackrestore.p0(%5) fastmath<contract> : (!fir.ref<i8>) -> ()
+      }
+    } to {
+      hlfir.yield %3#0 : !fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>> 
+    } user_defined_assign  (%arg1: !fir.ref<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>) to (%arg2: !fir.ref<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>) {
+      %5 = fir.embox %arg2 : (!fir.ref<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>) -> !fir.box<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>
+      %6 = fir.convert %5 : (!fir.box<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>) -> !fir.class<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>
+      %7 = fir.embox %arg1 : (!fir.ref<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>) -> !fir.box<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>
+      %8 = fir.convert %7 : (!fir.box<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>) -> !fir.class<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>
+      fir.call @_QMtypesPassign(%6, %8) fastmath<contract> : (!fir.class<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>, !fir.class<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>) -> ()
+    }
+  }
+  return
+}
+// CHECK-LABEL:   func.func @_QPtest3(
+// CHECK-SAME:                        %[[VAL_0:.*]]: !fir.ref<!fir.array<2xi32>> {fir.bindc_name = "y"}) {
+// CHECK:           %[[VAL_1:.*]] = fir.alloca !fir.box<!fir.heap<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>
+// CHECK:           %[[VAL_2:.*]] = fir.alloca i64
+// CHECK:           %[[VAL_3:.*]] = arith.constant 1 : i32
+// CHECK:           %[[VAL_4:.*]] = arith.constant 2 : index
+// CHECK:           %[[VAL_5:.*]] = fir.alloca !fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>> {bindc_name = ".result"}
+// CHECK:           %[[VAL_6:.*]] = fir.address_of(@_QFtest3Ex) : !fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>
+// CHECK:           %[[VAL_7:.*]] = fir.shape %[[VAL_4]] : (index) -> !fir.shape<1>
+// CHECK:           %[[VAL_8:.*]]:2 = hlfir.declare %[[VAL_6]](%[[VAL_7]]) {uniq_name = "_QFtest3Ex"} : (!fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>, !fir.shape<1>) -> (!fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>, !fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>)
+// CHECK:           %[[VAL_9:.*]]:2 = hlfir.declare %[[VAL_0]](%[[VAL_7]]) {uniq_name = "_QFtest3Ey"} : (!fir.ref<!fir.array<2xi32>>, !fir.shape<1>) -> (!fir.ref<!fir.array<2xi32>>, !fir.ref<!fir.array<2xi32>>)
+// CHECK:           %[[VAL_10:.*]] = hlfir.elemental %[[VAL_7]] unordered : (!fir.shape<1>) -> !hlfir.expr<2x!fir.logical<4>> {
+// CHECK:           ^bb0(%[[VAL_11:.*]]: index):
+// CHECK:             %[[VAL_12:.*]] = hlfir.designate %[[VAL_9]]#0 (%[[VAL_11]])  : (!fir.ref<!fir.array<2xi32>>, index) -> !fir.ref<i32>
+// CHECK:             %[[VAL_13:.*]] = fir.load %[[VAL_12]] : !fir.ref<i32>
+// CHECK:             %[[VAL_14:.*]] = arith.cmpi eq, %[[VAL_13]], %[[VAL_3]] : i32
+// CHECK:             %[[VAL_15:.*]] = fir.convert %[[VAL_14]] : (i1) -> !fir.logical<4>
+// CHECK:             hlfir.yield_element %[[VAL_15]] : !fir.logical<4>
+// CHECK:           }
+// CHECK:           %[[VAL_16:.*]]:3 = hlfir.associate %[[VAL_10]](%[[VAL_7]]) {uniq_name = ".tmp.where"} : (!hlfir.expr<2x!fir.logical<4>>, !fir.shape<1>) -> (!fir.ref<!fir.array<2x!fir.logical<4>>>, !fir.ref<!fir.array<2x!fir.logical<4>>>, i1)
+// CHECK:           hlfir.destroy %[[VAL_10]] : !hlfir.expr<2x!fir.logical<4>>
+// CHECK:           %[[VAL_18:.*]] = fir.call @llvm.stacksave.p0() fastmath<contract> : () -> !fir.ref<i8>
+// CHECK:           %[[VAL_19:.*]] = fir.call @_QPnew_obja() fastmath<contract> : () -> !fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>
+// CHECK:           fir.save_result %[[VAL_19]] to %[[VAL_5]](%[[VAL_7]]) : !fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>, !fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>, !fir.shape<1>
+// CHECK:           %[[VAL_20:.*]]:2 = hlfir.declare %[[VAL_5]](%[[VAL_7]]) {uniq_name = ".tmp.func_result"} : (!fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>, !fir.shape<1>) -> (!fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>, !fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>)
+// CHECK:           %[[VAL_27:.*]] = fir.call @_FortranACreateValueStack(%{{.*}}, %{{.*}}) : (!fir.ref<i8>, i32) -> !fir.llvm_ptr<i8>
+// CHECK:           fir.do_loop %[[VAL_28:.*]] = %{{.*}} to %[[VAL_4]] step %{{.*}} {
+// CHECK:             %[[VAL_29:.*]] = hlfir.designate %[[VAL_16]]#0 (%[[VAL_28]])  : (!fir.ref<!fir.array<2x!fir.logical<4>>>, index) -> !fir.ref<!fir.logical<4>>
+// CHECK:             %[[VAL_30:.*]] = fir.load %[[VAL_29]] : !fir.ref<!fir.logical<4>>
+// CHECK:             %[[VAL_31:.*]] = fir.convert %[[VAL_30]] : (!fir.logical<4>) -> i1
+// CHECK:             fir.if %[[VAL_31]] {
+// CHECK:               %[[VAL_32:.*]] = hlfir.designate %[[VAL_20]]#0 (%[[VAL_28]])  : (!fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>, index) -> !fir.ref<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>
+// CHECK:               %[[VAL_33:.*]] = fir.embox %[[VAL_32]] : (!fir.ref<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>) -> !fir.box<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>
+// CHECK:               %[[VAL_34:.*]] = fir.convert %[[VAL_33]] : (!fir.box<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>) -> !fir.box<none>
+// CHECK:               %[[VAL_35:.*]] = fir.call @_FortranAPushValue(%[[VAL_27]], %[[VAL_34]]) : (!fir.llvm_ptr<i8>, !fir.box<none>) -> none
+// CHECK:             }
+// CHECK:           }
+// CHECK:           fir.do_loop %[[VAL_37:.*]] = %{{.*}} to %[[VAL_4]] step %{{.*}} {
+// CHECK:             %[[VAL_38:.*]] = hlfir.designate %[[VAL_16]]#0 (%[[VAL_37]])  : (!fir.ref<!fir.array<2x!fir.logical<4>>>, index) -> !fir.ref<!fir.logical<4>>
+// CHECK:             %[[VAL_39:.*]] = fir.load %[[VAL_38]] : !fir.ref<!fir.logical<4>>
+// CHECK:             %[[VAL_40:.*]] = fir.convert %[[VAL_39]] : (!fir.logical<4>) -> i1
+// CHECK:             fir.if %[[VAL_40]] {
+// CHECK:               %[[VAL_41:.*]] = fir.load %[[VAL_2]] : !fir.ref<i64>
+// CHECK:               %[[VAL_42:.*]] = arith.addi %[[VAL_41]], %{{.*}} : i64
+// CHECK:               fir.store %[[VAL_42]] to %[[VAL_2]] : !fir.ref<i64>
+// CHECK:               %[[VAL_43:.*]] = fir.convert %[[VAL_1]] : (!fir.ref<!fir.box<!fir.heap<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>>) -> !fir.ref<!fir.box<none>>
+// CHECK:               %[[VAL_44:.*]] = fir.call @_FortranAValueAt(%[[VAL_27]], %[[VAL_41]], %[[VAL_43]]) : (!fir.llvm_ptr<i8>, i64, !fir.ref<!fir.box<none>>) -> none
+// CHECK:               %[[VAL_45:.*]] = fir.load %[[VAL_1]] : !fir.ref<!fir.box<!fir.heap<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>>
+// CHECK:               %[[VAL_46:.*]] = fir.box_addr %[[VAL_45]] : (!fir.box<!fir.heap<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>) -> !fir.heap<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>
+// CHECK:               %[[VAL_47:.*]] = hlfir.designate %[[VAL_8]]#0 (%[[VAL_37]])  : (!fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>, index) -> !fir.ref<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>
+// CHECK:               %[[VAL_48:.*]] = fir.convert %[[VAL_46]] : (!fir.heap<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>) -> !fir.ref<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>
+// CHECK:               %[[VAL_49:.*]] = fir.embox %[[VAL_47]] : (!fir.ref<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>) -> !fir.box<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>
+// CHECK:               %[[VAL_50:.*]] = fir.convert %[[VAL_49]] : (!fir.box<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>) -> !fir.class<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>
+// CHECK:               %[[VAL_51:.*]] = fir.embox %[[VAL_48]] : (!fir.ref<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>) -> !fir.box<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>
+// CHECK:               %[[VAL_52:.*]] = fir.convert %[[VAL_51]] : (!fir.box<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>) -> !fir.class<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>
+// CHECK:               fir.call @_QMtypesPassign(%[[VAL_50]], %[[VAL_52]]) fastmath<contract> : (!fir.class<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>, !fir.class<!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>) -> ()
+// CHECK:             }
+// CHECK:           }
+// CHECK:           hlfir.end_associate %[[VAL_16]]#1, %[[VAL_16]]#2 : !fir.ref<!fir.array<2x!fir.logical<4>>>, i1
+// CHECK:           %[[VAL_53:.*]] = fir.call @_FortranADestroyValueStack(%[[VAL_27]]) : (!fir.llvm_ptr<i8>) -> none
+// CHECK:           %[[VAL_54:.*]] = fir.embox %[[VAL_5]](%[[VAL_7]]) : (!fir.ref<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>, !fir.shape<1>) -> !fir.box<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>
+// CHECK:           %[[VAL_55:.*]] = fir.convert %[[VAL_54]] : (!fir.box<!fir.array<2x!fir.type<_QMtypesTud_assign{x:!fir.box<!fir.ptr<i32>>}>>>) -> !fir.box<none>
+// CHECK:           %[[VAL_56:.*]] = fir.call @_FortranADestroy(%[[VAL_55]]) fastmath<contract> : (!fir.box<none>) -> none
+// CHECK:           fir.call @llvm.stackrestore.p0(%[[VAL_18]]) fastmath<contract> : (!fir.ref<i8>) -> ()
+// CHECK:           return
+// CHECK:         }

Copy link
Contributor

@jeanPerier jeanPerier left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@vzakhari vzakhari merged commit 69d9ad1 into llvm:main Sep 20, 2023
4 checks passed
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.

None yet

3 participants