diff --git a/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp b/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp --- a/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp +++ b/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp @@ -379,9 +379,26 @@ /// expression bufferization at hlfir.end_associate. If there was more than one /// hlfir.end_associate, it would be cleaned up multiple times, perhaps before /// one of the other uses. +/// Note that we have to be careful about expressions used by a single +/// hlfir.end_associate that may be executed more times than the producer +/// of the expression value. This may also cause multiple clean-ups +/// for the same memory (e.g. cause double-free errors). For example, +/// hlfir.end_associate inside hlfir.elemental may cause such issues +/// for expressions produced outside of hlfir.elemental. static bool allOtherUsesAreSafeForAssociate(mlir::Value value, mlir::Operation *currentUse, mlir::Operation *endAssociate) { + // If value producer is from a different region than + // hlfir.associate/end_associate, then conservatively assume + // that the hlfir.end_associate may execute more times than + // the value producer. + // TODO: this may be improved for operations that cannot + // result in multiple executions (e.g. ifOp). + if (value.getParentRegion() != currentUse->getParentRegion() || + (endAssociate && + value.getParentRegion() != endAssociate->getParentRegion())) + return false; + for (mlir::Operation *useOp : value.getUsers()) if (!mlir::isa(useOp) && useOp != currentUse) { // hlfir.shape_of and hlfir.get_length will not disrupt cleanup so it is @@ -504,7 +521,7 @@ } // non-trivial value with more than one use. We will have to make a copy and // use that - hlfir::Entity source = hlfir::Entity{adaptor.getSource()}; + hlfir::Entity source = hlfir::Entity{bufferizedExpr}; auto [temp, cleanup] = createTempFromMold(loc, builder, source); builder.create(loc, source, temp, temp.isAllocatable(), /*keep_lhs_length_if_realloc=*/false, diff --git a/flang/test/HLFIR/associate-codegen.fir b/flang/test/HLFIR/associate-codegen.fir --- a/flang/test/HLFIR/associate-codegen.fir +++ b/flang/test/HLFIR/associate-codegen.fir @@ -366,3 +366,57 @@ func.func private @take_r4(!fir.ref) func.func private @take_l4(!fir.ref>) func.func private @take_c(!fir.boxchar<1>) + +// Test the hlfir.associate/hlfir.end_associate does not take ownership over +// a single-use hlfir.expr if hlfir.end_associate might be executed more times +// than the producer of hlfir.expr. This might cause double-free effects. +func.func @_QPtest_multiple_expr_uses_inside_elemental() { + %true = arith.constant true + %18 = fir.undefined !fir.heap> + %17 = fir.undefined index + %19:2 = hlfir.declare %18 typeparams %17 {uniq_name = ".tmp.intrinsic_result"} : (!fir.heap>, index) -> (!fir.boxchar<1>, !fir.heap>) + %20 = hlfir.as_expr %19#0 move %true : (!fir.boxchar<1>, i1) -> !hlfir.expr> + %21 = fir.undefined index + %22 = fir.shape %21 : (index) -> !fir.shape<1> + %23 = hlfir.elemental %22 unordered : (!fir.shape<1>) -> !hlfir.expr> { + ^bb0(%arg2: index): + %35:3 = hlfir.associate %20 typeparams %17 {uniq_name = "adapt.valuebyref"} : (!hlfir.expr>, index) -> (!fir.boxchar<1>, !fir.ref>, i1) + hlfir.end_associate %35#1, %35#2 : !fir.ref>, i1 + %ci1 = arith.constant 1 : i1 + %42 = fir.convert %ci1 : (i1) -> !fir.logical<4> + hlfir.yield_element %42 : !fir.logical<4> + } + return +} +// CHECK-LABEL: func.func @_QPtest_multiple_expr_uses_inside_elemental() { +// CHECK: %[[VAL_2:.*]] = arith.constant true +// CHECK: %[[VAL_3:.*]] = fir.undefined !fir.heap> +// CHECK: %[[VAL_4:.*]] = fir.undefined index +// CHECK: %[[VAL_5:.*]]:2 = hlfir.declare %[[VAL_3]] typeparams %[[VAL_4]] {uniq_name = ".tmp.intrinsic_result"} : (!fir.heap>, index) -> (!fir.boxchar<1>, !fir.heap>) +// CHECK: %[[VAL_6:.*]] = fir.undefined tuple, i1> +// CHECK: %[[VAL_7:.*]] = fir.insert_value %[[VAL_6]], %[[VAL_2]], [1 : index] : (tuple, i1>, i1) -> tuple, i1> +// CHECK: %[[VAL_8:.*]] = fir.insert_value %[[VAL_7]], %[[VAL_5]]#0, [0 : index] : (tuple, i1>, !fir.boxchar<1>) -> tuple, i1> +// CHECK: %[[VAL_9:.*]] = fir.undefined index +// CHECK: %[[VAL_10:.*]] = fir.shape %[[VAL_9]] : (index) -> !fir.shape<1> +// CHECK: %[[VAL_11:.*]] = fir.allocmem !fir.array>, %[[VAL_9]] {bindc_name = ".tmp.array", uniq_name = ""} +// CHECK: %[[VAL_12:.*]]:2 = hlfir.declare %[[VAL_11]](%[[VAL_10]]) {uniq_name = ".tmp.array"} : (!fir.heap>>, !fir.shape<1>) -> (!fir.box>>, !fir.heap>>) +// CHECK: %[[VAL_13:.*]] = arith.constant true +// CHECK: %[[VAL_14:.*]] = arith.constant 1 : index +// CHECK: fir.do_loop %[[VAL_15:.*]] = %[[VAL_14]] to %[[VAL_9]] step %[[VAL_14]] unordered { +// CHECK: %[[VAL_16:.*]] = fir.alloca !fir.char<1,?>(%[[VAL_4]] : index) {bindc_name = ".tmp"} +// CHECK: %[[VAL_17:.*]] = arith.constant false +// CHECK: %[[VAL_18:.*]]:2 = hlfir.declare %[[VAL_16]] typeparams %[[VAL_4]] {uniq_name = ".tmp"} : (!fir.ref>, index) -> (!fir.boxchar<1>, !fir.ref>) +// CHECK: hlfir.assign %[[VAL_5]]#0 to %[[VAL_18]]#0 temporary_lhs : !fir.boxchar<1>, !fir.boxchar<1> +// CHECK: %[[VAL_19:.*]] = fir.undefined tuple, i1> +// CHECK: %[[VAL_20:.*]] = fir.insert_value %[[VAL_19]], %[[VAL_17]], [1 : index] : (tuple, i1>, i1) -> tuple, i1> +// CHECK: %[[VAL_21:.*]] = fir.insert_value %[[VAL_20]], %[[VAL_18]]#0, [0 : index] : (tuple, i1>, !fir.boxchar<1>) -> tuple, i1> +// CHECK: %[[VAL_22:.*]] = arith.constant true +// CHECK: %[[VAL_23:.*]] = fir.convert %[[VAL_22]] : (i1) -> !fir.logical<4> +// CHECK: %[[VAL_24:.*]] = hlfir.designate %[[VAL_12]]#0 (%[[VAL_15]]) : (!fir.box>>, index) -> !fir.ref> +// CHECK: hlfir.assign %[[VAL_23]] to %[[VAL_24]] temporary_lhs : !fir.logical<4>, !fir.ref> +// CHECK: } +// CHECK: %[[VAL_25:.*]] = fir.undefined tuple>>, i1> +// CHECK: %[[VAL_26:.*]] = fir.insert_value %[[VAL_25]], %[[VAL_13]], [1 : index] : (tuple>>, i1>, i1) -> tuple>>, i1> +// CHECK: %[[VAL_27:.*]] = fir.insert_value %[[VAL_26]], %[[VAL_12]]#0, [0 : index] : (tuple>>, i1>, !fir.box>>) -> tuple>>, i1> +// CHECK: return +// CHECK: }