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] Fix hlfir.as_expr codegen for polymorphic entities #80824

Merged
merged 2 commits into from
Feb 7, 2024

Conversation

jeanPerier
Copy link
Contributor

#80683 revealed that hlfir.as_expr was propagating the temporary buffer for polymorphic values as an allocatable while codegen later expects to be working with fir.box/fir.class but not fir.ref<box/class> when processing the operations using the hlfir.as_expr result.

Dereference the temporary allocatable as soon as it is created.

llvm#80683 revealed that
hlfir.as_expr was propagating the temporary buffer for polymorphic
values as an allocatable while further codegen on the result uses
expect to be working with fir.box/fir.class but not fir.ref<box/class>.

Dereference the temporary allocatable as soon as it is created.
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Feb 6, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 6, 2024

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

Author: None (jeanPerier)

Changes

#80683 revealed that hlfir.as_expr was propagating the temporary buffer for polymorphic values as an allocatable while codegen later expects to be working with fir.box/fir.class but not fir.ref<box/class> when processing the operations using the hlfir.as_expr result.

Dereference the temporary allocatable as soon as it is created.


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

3 Files Affected:

  • (modified) flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp (+18-12)
  • (added) flang/test/HLFIR/as_expr-codegen-polymorphic.fir (+38)
  • (modified) flang/test/HLFIR/bufferize-poly-expr.fir (+12-13)
diff --git a/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp b/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp
index 15258e3e9c309d..2eaed166190a5d 100644
--- a/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp
+++ b/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp
@@ -159,6 +159,22 @@ createArrayTemp(mlir::Location loc, fir::FirOpBuilder &builder,
   return {hlfir::Entity{declareOp.getBase()}, trueVal};
 }
 
+/// Copy \p source into a new temporary and package the temporary into a
+/// <temp,cleanup> tuple. The temporary may be heap of stack allocated.
+static mlir::Value copyInTempAndPackage(mlir::Location loc,
+                                        fir::FirOpBuilder &builder,
+                                        hlfir::Entity source) {
+  auto [temp, cleanup] = hlfir::createTempFromMold(loc, builder, source);
+  builder.create<hlfir::AssignOp>(loc, source, temp, temp.isAllocatable(),
+                                  /*keep_lhs_length_if_realloc=*/false,
+                                  /*temporary_lhs=*/true);
+  // Dereference allocatable temporary directly to simplify processing
+  // of its uses.
+  if (temp.isAllocatable())
+    temp = hlfir::derefPointersAndAllocatables(loc, builder, temp);
+  return packageBufferizedExpr(loc, builder, temp, cleanup);
+}
+
 struct AsExprOpConversion : public mlir::OpConversionPattern<hlfir::AsExprOp> {
   using mlir::OpConversionPattern<hlfir::AsExprOp>::OpConversionPattern;
   explicit AsExprOpConversion(mlir::MLIRContext *ctx)
@@ -178,12 +194,7 @@ struct AsExprOpConversion : public mlir::OpConversionPattern<hlfir::AsExprOp> {
     }
     // Otherwise, create a copy in a new buffer.
     hlfir::Entity source = hlfir::Entity{adaptor.getVar()};
-    auto [temp, cleanup] = hlfir::createTempFromMold(loc, builder, source);
-    builder.create<hlfir::AssignOp>(loc, source, temp, temp.isAllocatable(),
-                                    /*keep_lhs_length_if_realloc=*/false,
-                                    /*temporary_lhs=*/true);
-    mlir::Value bufferizedExpr =
-        packageBufferizedExpr(loc, builder, temp, cleanup);
+    mlir::Value bufferizedExpr = copyInTempAndPackage(loc, builder, source);
     rewriter.replaceOp(asExpr, bufferizedExpr);
     return mlir::success();
   }
@@ -542,12 +553,7 @@ struct AssociateOpConversion
     // non-trivial value with more than one use. We will have to make a copy and
     // use that
     hlfir::Entity source = hlfir::Entity{bufferizedExpr};
-    auto [temp, cleanup] = hlfir::createTempFromMold(loc, builder, source);
-    builder.create<hlfir::AssignOp>(loc, source, temp, temp.isAllocatable(),
-                                    /*keep_lhs_length_if_realloc=*/false,
-                                    /*temporary_lhs=*/true);
-    mlir::Value bufferTuple =
-        packageBufferizedExpr(loc, builder, temp, cleanup);
+    mlir::Value bufferTuple = copyInTempAndPackage(loc, builder, source);
     bufferizedExpr = getBufferizedExprStorage(bufferTuple);
     replaceWith(bufferizedExpr, hlfir::Entity{bufferizedExpr}.getFirBase(),
                 getBufferizedExprMustFreeFlag(bufferTuple));
diff --git a/flang/test/HLFIR/as_expr-codegen-polymorphic.fir b/flang/test/HLFIR/as_expr-codegen-polymorphic.fir
new file mode 100644
index 00000000000000..30a8da72c3e486
--- /dev/null
+++ b/flang/test/HLFIR/as_expr-codegen-polymorphic.fir
@@ -0,0 +1,38 @@
+// Test hlfir.as_expr codegen for polymorphic expressions.
+
+// RUN: fir-opt %s -bufferize-hlfir | FileCheck %s
+
+!t = !fir.type<t{i:i32}>
+func.func @as_expr_class(%arg0 : !fir.class<!t>, %arg1: !fir.ref<!t>) {
+    %0 = hlfir.as_expr %arg0 : (!fir.class<!t>) -> !hlfir.expr<!t?>
+    hlfir.assign %0 to %arg1 : !hlfir.expr<!t?>, !fir.ref<!t>
+   return
+}
+// CHECK-LABEL:   func.func @as_expr_class(
+// CHECK:           %[[VAL_5:.*]] = arith.constant true
+// CHECK:           %[[VAL_6:.*]]:2 = hlfir.declare %{{.*}} {fortran_attrs = #fir.var_attrs<allocatable>, uniq_name = ".tmp"} : (!fir.ref<!fir.class<!fir.heap<!fir.type<t{i:i32}>>>>) -> (!fir.ref<!fir.class<!fir.heap<!fir.type<t{i:i32}>>>>, !fir.ref<!fir.class<!fir.heap<!fir.type<t{i:i32}>>>>)
+// ... copy ...
+// CHECK:           %[[VAL_11:.*]] = fir.load %[[VAL_6]]#0 : !fir.ref<!fir.class<!fir.heap<!fir.type<t{i:i32}>>>>
+// CHECK:           %[[VAL_12:.*]] = fir.undefined tuple<!fir.class<!fir.heap<!fir.type<t{i:i32}>>>, i1>
+// CHECK:           %[[VAL_13:.*]] = fir.insert_value %[[VAL_12]], %[[VAL_5]], [1 : index] : (tuple<!fir.class<!fir.heap<!fir.type<t{i:i32}>>>, i1>, i1) -> tuple<!fir.class<!fir.heap<!fir.type<t{i:i32}>>>, i1>
+// CHECK:           %[[VAL_14:.*]] = fir.insert_value %[[VAL_13]], %[[VAL_11]], [0 : index] : (tuple<!fir.class<!fir.heap<!fir.type<t{i:i32}>>>, i1>, !fir.class<!fir.heap<!fir.type<t{i:i32}>>>) -> tuple<!fir.class<!fir.heap<!fir.type<t{i:i32}>>>, i1>
+// CHECK:           hlfir.assign %[[VAL_11]] to %{{.*}} : !fir.class<!fir.heap<!fir.type<t{i:i32}>>>, !fir.ref<!fir.type<t{i:i32}>>
+
+
+func.func @as_expr_class_2(%arg0 : !fir.class<!fir.array<?x!t>>) {
+    %0 = hlfir.as_expr %arg0 : (!fir.class<!fir.array<?x!t>>) -> !hlfir.expr<?x!t?>
+    %c1 = arith.constant 1 : index
+    %1 = hlfir.apply %0, %c1 : (!hlfir.expr<?x!t?>, index) -> !hlfir.expr<!t?>
+   return
+}
+// CHECK-LABEL:   func.func @as_expr_class_2(
+// CHECK:           %[[VAL_9:.*]] = arith.constant true
+// CHECK:           %[[VAL_10:.*]]:2 = hlfir.declare %{{.*}} {fortran_attrs = #fir.var_attrs<allocatable>, uniq_name = ".tmp"} : (!fir.ref<!fir.class<!fir.heap<!fir.array<?x!fir.type<t{i:i32}>>>>>) -> (!fir.ref<!fir.class<!fir.heap<!fir.array<?x!fir.type<t{i:i32}>>>>>, !fir.ref<!fir.class<!fir.heap<!fir.array<?x!fir.type<t{i:i32}>>>>>)
+// CHECK:           %[[VAL_11:.*]] = arith.constant 1 : i32
+// ... copy ...
+// CHECK:           %[[VAL_15:.*]] = fir.load %[[VAL_10]]#0 : !fir.ref<!fir.class<!fir.heap<!fir.array<?x!fir.type<t{i:i32}>>>>>
+// CHECK:           %[[VAL_16:.*]] = fir.undefined tuple<!fir.class<!fir.heap<!fir.array<?x!fir.type<t{i:i32}>>>>, i1>
+// CHECK:           %[[VAL_17:.*]] = fir.insert_value %[[VAL_16]], %[[VAL_9]], [1 : index] : (tuple<!fir.class<!fir.heap<!fir.array<?x!fir.type<t{i:i32}>>>>, i1>, i1) -> tuple<!fir.class<!fir.heap<!fir.array<?x!fir.type<t{i:i32}>>>>, i1>
+// CHECK:           %[[VAL_18:.*]] = fir.insert_value %[[VAL_17]], %[[VAL_15]], [0 : index] : (tuple<!fir.class<!fir.heap<!fir.array<?x!fir.type<t{i:i32}>>>>, i1>, !fir.class<!fir.heap<!fir.array<?x!fir.type<t{i:i32}>>>>) -> tuple<!fir.class<!fir.heap<!fir.array<?x!fir.type<t{i:i32}>>>>, i1>
+// CHECK:           %[[VAL_19:.*]] = arith.constant 1 : index
+// CHECK:           %[[VAL_20:.*]] = hlfir.designate %[[VAL_15]] (%[[VAL_19]])  : (!fir.class<!fir.heap<!fir.array<?x!fir.type<t{i:i32}>>>>, index) -> !fir.class<!fir.type<t{i:i32}>>
diff --git a/flang/test/HLFIR/bufferize-poly-expr.fir b/flang/test/HLFIR/bufferize-poly-expr.fir
index df43c74b2aaccb..dfa62a9ac5ab72 100644
--- a/flang/test/HLFIR/bufferize-poly-expr.fir
+++ b/flang/test/HLFIR/bufferize-poly-expr.fir
@@ -26,12 +26,12 @@ func.func @test_poly_expr_without_associate() {
 // CHECK:           %[[VAL_11:.*]] = fir.convert %[[VAL_4]]#1 : (!fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>) -> !fir.box<none>
 // CHECK:           %[[VAL_12:.*]] = fir.call @_FortranAAllocatableApplyMold(%[[VAL_10]], %[[VAL_11]], %[[VAL_9]]) : (!fir.ref<!fir.box<none>>, !fir.box<none>, i32) -> none
 // CHECK:           hlfir.assign %[[VAL_4]]#0 to %[[VAL_8]]#0 realloc temporary_lhs : !fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>, !fir.ref<!fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>>
-// CHECK:           %[[VAL_13:.*]] = fir.undefined tuple<!fir.ref<!fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>>, i1>
-// CHECK:           %[[VAL_14:.*]] = fir.insert_value %[[VAL_13]], %[[VAL_7]], [1 : index] : (tuple<!fir.ref<!fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>>, i1>, i1) -> tuple<!fir.ref<!fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>>, i1>
-// CHECK:           %[[VAL_15:.*]] = fir.insert_value %[[VAL_14]], %[[VAL_8]]#0, [0 : index] : (tuple<!fir.ref<!fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>>, i1>, !fir.ref<!fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>>) -> tuple<!fir.ref<!fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>>, i1>
-// CHECK:           hlfir.assign %[[VAL_8]]#0 to %[[VAL_2]]#0 realloc : !fir.ref<!fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>>, !fir.ref<!fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>>
-// CHECK:           %[[VAL_16:.*]] = fir.load %[[VAL_8]]#0 : !fir.ref<!fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>>
-// CHECK:           %[[VAL_17:.*]] = fir.box_addr %[[VAL_16]] : (!fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>) -> !fir.heap<!fir.type<_QFtestTt{c:i32}>>
+// CHECK:           %[[VAL_8B:.*]] = fir.load %[[VAL_8]]#0
+// CHECK:           %[[VAL_13:.*]] = fir.undefined tuple<!fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>, i1>
+// CHECK:           %[[VAL_14:.*]] = fir.insert_value %[[VAL_13]], %[[VAL_7]], [1 : index] : (tuple<!fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>, i1>, i1) -> tuple<!fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>, i1>
+// CHECK:           %[[VAL_15:.*]] = fir.insert_value %[[VAL_14]], %[[VAL_8B]], [0 : index] : (tuple<!fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>, i1>, !fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>) -> tuple<!fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>, i1>
+// CHECK:           hlfir.assign %[[VAL_8B]] to %[[VAL_2]]#0 realloc : !fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>, !fir.ref<!fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>>
+// CHECK:           %[[VAL_17:.*]] = fir.box_addr %[[VAL_8B]] : (!fir.class<!fir.heap<!fir.type<_QFtestTt{c:i32}>>>) -> !fir.heap<!fir.type<_QFtestTt{c:i32}>>
 // CHECK:           fir.freemem %[[VAL_17]] : !fir.heap<!fir.type<_QFtestTt{c:i32}>>
 // CHECK:           return
 // CHECK:         }
@@ -81,14 +81,13 @@ func.func @test_poly_expr_with_associate(%arg1: !fir.class<!fir.array<3x!fir.typ
 // CHECK:           %[[VAL_17:.*]] = fir.convert %[[VAL_5]] : (!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>) -> !fir.box<none>
 // CHECK:           %[[VAL_18:.*]] = fir.call @_FortranAAllocatableApplyMold(%[[VAL_16]], %[[VAL_17]], %[[VAL_15]]) : (!fir.ref<!fir.box<none>>, !fir.box<none>, i32) -> none
 // CHECK:           hlfir.assign %[[VAL_5]] to %[[VAL_14]]#0 realloc temporary_lhs : !fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>, !fir.ref<!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>>
-// CHECK:           %[[VAL_19:.*]] = fir.undefined tuple<!fir.ref<!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>>, i1>
-// CHECK:           %[[VAL_20:.*]] = fir.insert_value %[[VAL_19]], %[[VAL_13]], [1 : index] : (tuple<!fir.ref<!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>>, i1>, i1) -> tuple<!fir.ref<!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>>, i1>
-// CHECK:           %[[VAL_21:.*]] = fir.insert_value %[[VAL_20]], %[[VAL_14]]#0, [0 : index] : (tuple<!fir.ref<!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>>, i1>, !fir.ref<!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>>) -> tuple<!fir.ref<!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>>, i1>
+// CHECK:           %[[VAL_14B:.*]] = fir.load %[[VAL_14]]#0
+// CHECK:           %[[VAL_19:.*]] = fir.undefined tuple<!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>, i1>
+// CHECK:           %[[VAL_20:.*]] = fir.insert_value %[[VAL_19]], %[[VAL_13]], [1 : index] : (tuple<!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>, i1>, i1) -> tuple<!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>, i1>
+// CHECK:           %[[VAL_21:.*]] = fir.insert_value %[[VAL_20]], %[[VAL_14B]], [0 : index] : (tuple<!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>, i1>, !fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>) -> tuple<!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>, i1>
 // CHECK:           %[[VAL_22:.*]] = arith.constant 0 : index
 // CHECK:           %[[VAL_23:.*]]:3 = fir.box_dims %[[VAL_5]], %[[VAL_22]] : (!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>, index) -> (index, index, index)
 // CHECK:           %[[VAL_24:.*]] = fir.shape %[[VAL_23]]#1 : (index) -> !fir.shape<1>
-// CHECK:           %[[VAL_25:.*]] = fir.load %[[VAL_14]]#0 : !fir.ref<!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>>
-// CHECK:           %[[VAL_26:.*]] = fir.load %[[VAL_14]]#1 : !fir.ref<!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>>
 // CHECK:           %[[VAL_27:.*]] = fir.convert %[[VAL_2]] : (!fir.ref<!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>>) -> !fir.box<none>
 // CHECK:           %[[VAL_28:.*]] = fir.call @_FortranADestroy(%[[VAL_27]]) fastmath<contract> : (!fir.box<none>) -> none
 // CHECK:           %[[VAL_29:.*]] = arith.constant 3 : index
@@ -96,10 +95,10 @@ func.func @test_poly_expr_with_associate(%arg1: !fir.class<!fir.array<3x!fir.typ
 // CHECK:           %[[VAL_31:.*]] = arith.constant 1 : index
 // CHECK:           fir.do_loop %[[VAL_32:.*]] = %[[VAL_31]] to %[[VAL_29]] step %[[VAL_31]] {
 // CHECK:             %[[VAL_33:.*]] = hlfir.designate %[[VAL_3]]#0 (%[[VAL_32]])  : (!fir.class<!fir.array<3x!fir.type<_QMtest_typeTt1{i:i32}>>>, index) -> !fir.class<!fir.type<_QMtest_typeTt1{i:i32}>>
-// CHECK:             %[[VAL_34:.*]] = hlfir.designate %[[VAL_25]] (%[[VAL_32]])  : (!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>, index) -> !fir.class<!fir.type<_QMtest_typeTt1{i:i32}>>
+// CHECK:             %[[VAL_34:.*]] = hlfir.designate %[[VAL_14B]] (%[[VAL_32]])  : (!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>, index) -> !fir.class<!fir.type<_QMtest_typeTt1{i:i32}>>
 // CHECK:             fir.dispatch "assign"(%[[VAL_33]] : !fir.class<!fir.type<_QMtest_typeTt1{i:i32}>>) (%[[VAL_33]], %[[VAL_34]] : !fir.class<!fir.type<_QMtest_typeTt1{i:i32}>>, !fir.class<!fir.type<_QMtest_typeTt1{i:i32}>>) {pass_arg_pos = 0 : i32}
 // CHECK:           }
-// CHECK:           %[[VAL_35:.*]] = fir.box_addr %[[VAL_26]] : (!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>) -> !fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>
+// CHECK:           %[[VAL_35:.*]] = fir.box_addr %[[VAL_14B]] : (!fir.class<!fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>>) -> !fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>
 // CHECK:           fir.freemem %[[VAL_35]] : !fir.heap<!fir.array<?x!fir.type<_QMtest_typeTt1{i:i32}>>>
 // CHECK:           return
 // CHECK:         }

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.

Thank you for fixing it, Jean!

flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp Outdated Show resolved Hide resolved
Co-authored-by: Slava Zakharin <szakharin@nvidia.com>
@jeanPerier jeanPerier merged commit 0f439f3 into llvm:main Feb 7, 2024
4 checks passed
@jeanPerier jeanPerier deleted the jp-fix-hlfir-bug branch February 7, 2024 09:50
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.

3 participants