[flang] Avoid double free in bufferize pass

[clementval]

In some cases where we have an hlfir.no_reassoc operation, the bufferization pass could not earse the hlfir.destroy op during the hlfir.associate op conversion as show in the example below.

func.func @double_free(%arg0: !fir.boxchar<1>) {
  %c5 = arith.constant 5 : index
  %true = arith.constant true
  %0 = hlfir.as_expr %arg0 move %true : (!fir.boxchar<1>, i1) -> !hlfir.expr<!fir.char<1,?>>
  %1 = hlfir.no_reassoc %0 : !hlfir.expr<!fir.char<1,?>>
  %2:3 = hlfir.associate %1 typeparams %c5 {adapt.valuebyref} : (!hlfir.expr<!fir.char<1,?>>, index) -> (!fir.boxchar<1>, !fir.ref<!fir.char<1,?>>, i1)
  fir.call @noop(%2#0) : (!fir.boxchar<1>) -> ()
  hlfir.end_associate %2#1, %2#2 : !fir.ref<!fir.char<1,?>>, i1
  hlfir.destroy %0 : !hlfir.expr<!fir.char<1,?>>
  return
} 
func.func private @noop(!fir.boxchar<1>)

The bufferization pass is looking at uses of its source %1 that is the result of an hlfir.no_reassoc operation. In order to avoid double free generation, also look at the indirection in presence of hlfir.no_reassoc.

[llvmbot]

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

Author: Valentin Clement (バレンタイン クレメン) (clementval)

Changes

In some cases where we have an hlfir.no_reassoc operation, the bufferization pass could not earse the hlfir.destroy op during the hlfir.associate op conversion as show in the example below.

func.func @<!-- -->double_free(%arg0: !fir.boxchar&lt;1&gt;) {
  %c5 = arith.constant 5 : index
  %true = arith.constant true
  %0 = hlfir.as_expr %arg0 move %true : (!fir.boxchar&lt;1&gt;, i1) -&gt; !hlfir.expr&lt;!fir.char&lt;1,?&gt;&gt;
  %1 = hlfir.no_reassoc %0 : !hlfir.expr&lt;!fir.char&lt;1,?&gt;&gt;
  %2:3 = hlfir.associate %1 typeparams %c5 {adapt.valuebyref} : (!hlfir.expr&lt;!fir.char&lt;1,?&gt;&gt;, index) -&gt; (!fir.boxchar&lt;1&gt;, !fir.ref&lt;!fir.char&lt;1,?&gt;&gt;, i1)
  fir.call @<!-- -->noop(%2#<!-- -->0) : (!fir.boxchar&lt;1&gt;) -&gt; ()
  hlfir.end_associate %2#<!-- -->1, %2#<!-- -->2 : !fir.ref&lt;!fir.char&lt;1,?&gt;&gt;, i1
  hlfir.destroy %0 : !hlfir.expr&lt;!fir.char&lt;1,?&gt;&gt;
  return
} 
func.func private @<!-- -->noop(!fir.boxchar&lt;1&gt;)

The bufferization pass is looking at uses of its source %1 that is the result of an hlfir.no_reassoc operation. In order to avoid double free generation, also look at the indirection in presence of hlfir.no_reassoc.

---

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

2 Files Affected:

• (modified) flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp (+5)
• (modified) flang/test/HLFIR/bufferize01.fir (+28)

diff --git a/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp b/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp
index dd4f4c42414f3..e292b562763cc 100644
--- a/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp
+++ b/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp
@@ -534,6 +534,11 @@ struct AssociateOpConversion
       mlir::Value firBase = hlfir::Entity{bufferizedExpr}.getFirBase();
       replaceWith(bufferizedExpr, firBase, mustFree);
       eraseAllUsesInDestroys(associate.getSource(), rewriter);
+      // Make sure to erase the hlfir.destroy if there is an indirection through
+      // a hlfir.no_reassoc operation.
+      if (auto noReassoc = mlir::dyn_cast_or_null<hlfir::NoReassocOp>(
+              associate.getSource().getDefiningOp()))
+        eraseAllUsesInDestroys(noReassoc.getVal(), rewriter);
       return mlir::success();
     }
     if (isTrivialValue) {
diff --git a/flang/test/HLFIR/bufferize01.fir b/flang/test/HLFIR/bufferize01.fir
index 89af0c2766f94..02ac6076268af 100644
--- a/flang/test/HLFIR/bufferize01.fir
+++ b/flang/test/HLFIR/bufferize01.fir
@@ -143,3 +143,31 @@ fir.global linkonce @_QQclXce30ef70ff16a711a97719fb946c0b3d constant : !fir.char
   fir.has_value %0 : !fir.char<1,1>
 }
 func.func private @_FortranAPushArrayConstructorValue(!fir.llvm_ptr<i8>, !fir.box<none>) -> none attributes {fir.runtime}
+
+// -----
+
+// Test that only a single freemem is generated.
+
+func.func @double_free(%arg0: !fir.boxchar<1>) {
+  %c5 = arith.constant 5 : index
+  %true = arith.constant true
+  %0 = hlfir.as_expr %arg0 move %true : (!fir.boxchar<1>, i1) -> !hlfir.expr<!fir.char<1,?>>
+  %1 = hlfir.no_reassoc %0 : !hlfir.expr<!fir.char<1,?>>
+  %2:3 = hlfir.associate %1 typeparams %c5 {adapt.valuebyref} : (!hlfir.expr<!fir.char<1,?>>, index) -> (!fir.boxchar<1>, !fir.ref<!fir.char<1,?>>, i1)
+  fir.call @noop(%2#0) : (!fir.boxchar<1>) -> ()
+  hlfir.end_associate %2#1, %2#2 : !fir.ref<!fir.char<1,?>>, i1
+  hlfir.destroy %0 : !hlfir.expr<!fir.char<1,?>>
+  return
+} 
+func.func private @noop(!fir.boxchar<1>)
+
+// CHECK-LABEL: func.func @double_free(
+// CHECK-SAME:  %[[ARG0:.*]]: !fir.boxchar<1>) {
+// CHECK: %[[NO_REASSOC:.*]] = hlfir.no_reassoc %[[ARG0]] : !fir.boxchar<1>
+// CHECK: %[[BOX_ADDR:.*]] = fir.box_addr %[[NO_REASSOC]] : (!fir.boxchar<1>) -> !fir.ref<!fir.char<1,?>>
+// CHECK: fir.call @noop
+// CHECK: %[[CONV:.*]] = fir.convert %[[BOX_ADDR]] : (!fir.ref<!fir.char<1,?>>) -> !fir.heap<!fir.char<1,?>>
+// CHECK: fir.freemem %[[CONV]] : !fir.heap<!fir.char<1,?>>
+// CHECK-NOT: fir.freemem
+
+

[tblah]

Thanks! LGTM