[flang] Allow user to define free via BIND(C)

[jeanPerier]

A user defining and using free/malloc via BIND(C) would previously cause flang to crash when generating LLVM IR with error "redefinition of symbol named 'free'". This was caused by flang codegen not expecting to find a mlir::func::FuncOp definition of these function and emitting a new mlir::LLVM::FuncOp that later conflicted when translating the mlir::func::FuncOp.

[llvmbot]

@llvm/pr-subscribers-flang-codegen

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

Author: None (jeanPerier)

Changes

A user defining and using free/malloc via BIND(C) would previously cause flang to crash when generating LLVM IR with error "redefinition of symbol named 'free'". This was caused by flang codegen not expecting to find a mlir::func::FuncOp definition of these function and emitting a new mlir::LLVM::FuncOp that later conflicted when translating the mlir::func::FuncOp.

---

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

2 Files Affected:

• (modified) flang/lib/Optimizer/CodeGen/CodeGen.cpp (+24-17)
• (added) flang/test/Fir/already-defined-free.fir (+22)

diff --git a/flang/lib/Optimizer/CodeGen/CodeGen.cpp b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
index f2c731d47909a94..33f64fa5286c0eb 100644
--- a/flang/lib/Optimizer/CodeGen/CodeGen.cpp
+++ b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
@@ -1156,20 +1156,23 @@ struct EmboxCharOpConversion : public FIROpConversion<fir::EmboxCharOp> {
 } // namespace
 
 /// Return the LLVMFuncOp corresponding to the standard malloc call.
-static mlir::LLVM::LLVMFuncOp
+static mlir::SymbolRefAttr
 getMalloc(fir::AllocMemOp op, mlir::ConversionPatternRewriter &rewriter) {
+  static constexpr char mallocName[] = "malloc";
   auto module = op->getParentOfType<mlir::ModuleOp>();
-  if (mlir::LLVM::LLVMFuncOp mallocFunc =
-          module.lookupSymbol<mlir::LLVM::LLVMFuncOp>("malloc"))
-    return mallocFunc;
+  if (auto mallocFunc = module.lookupSymbol<mlir::LLVM::LLVMFuncOp>(mallocName))
+    return mlir::SymbolRefAttr::get(mallocFunc);
+  if (auto userMalloc = module.lookupSymbol<mlir::func::FuncOp>(mallocName))
+    return mlir::SymbolRefAttr::get(userMalloc);
   mlir::OpBuilder moduleBuilder(
       op->getParentOfType<mlir::ModuleOp>().getBodyRegion());
   auto indexType = mlir::IntegerType::get(op.getContext(), 64);
-  return moduleBuilder.create<mlir::LLVM::LLVMFuncOp>(
-      rewriter.getUnknownLoc(), "malloc",
+  auto mallocDecl = moduleBuilder.create<mlir::LLVM::LLVMFuncOp>(
+      op.getLoc(), mallocName,
       mlir::LLVM::LLVMFunctionType::get(getLlvmPtrType(op.getContext()),
                                         indexType,
                                         /*isVarArg=*/false));
+  return mlir::SymbolRefAttr::get(mallocDecl);
 }
 
 /// Helper function for generating the LLVM IR that computes the distance
@@ -1217,7 +1220,6 @@ struct AllocMemOpConversion : public FIROpConversion<fir::AllocMemOp> {
   matchAndRewrite(fir::AllocMemOp heap, OpAdaptor adaptor,
                   mlir::ConversionPatternRewriter &rewriter) const override {
     mlir::Type heapTy = heap.getType();
-    mlir::LLVM::LLVMFuncOp mallocFunc = getMalloc(heap, rewriter);
     mlir::Location loc = heap.getLoc();
     auto ity = lowerTy().indexType();
     mlir::Type dataTy = fir::unwrapRefType(heapTy);
@@ -1230,7 +1232,7 @@ struct AllocMemOpConversion : public FIROpConversion<fir::AllocMemOp> {
     for (mlir::Value opnd : adaptor.getOperands())
       size = rewriter.create<mlir::LLVM::MulOp>(
           loc, ity, size, integerCast(loc, rewriter, ity, opnd));
-    heap->setAttr("callee", mlir::SymbolRefAttr::get(mallocFunc));
+    heap->setAttr("callee", getMalloc(heap, rewriter));
     rewriter.replaceOpWithNewOp<mlir::LLVM::CallOp>(
         heap, ::getLlvmPtrType(heap.getContext()), size, heap->getAttrs());
     return mlir::success();
@@ -1248,19 +1250,25 @@ struct AllocMemOpConversion : public FIROpConversion<fir::AllocMemOp> {
 } // namespace
 
 /// Return the LLVMFuncOp corresponding to the standard free call.
-static mlir::LLVM::LLVMFuncOp
-getFree(fir::FreeMemOp op, mlir::ConversionPatternRewriter &rewriter) {
+static mlir::SymbolRefAttr getFree(fir::FreeMemOp op,
+                                   mlir::ConversionPatternRewriter &rewriter) {
+  static constexpr char freeName[] = "free";
   auto module = op->getParentOfType<mlir::ModuleOp>();
-  if (mlir::LLVM::LLVMFuncOp freeFunc =
-          module.lookupSymbol<mlir::LLVM::LLVMFuncOp>("free"))
-    return freeFunc;
+  // Check if free already defined in the module.
+  if (auto freeFunc = module.lookupSymbol<mlir::LLVM::LLVMFuncOp>(freeName))
+    return mlir::SymbolRefAttr::get(freeFunc);
+  if (auto freeDefinedByUser =
+          module.lookupSymbol<mlir::func::FuncOp>(freeName))
+    return mlir::SymbolRefAttr::get(freeDefinedByUser);
+  // Create llvm declaration for free.
   mlir::OpBuilder moduleBuilder(module.getBodyRegion());
   auto voidType = mlir::LLVM::LLVMVoidType::get(op.getContext());
-  return moduleBuilder.create<mlir::LLVM::LLVMFuncOp>(
-      rewriter.getUnknownLoc(), "free",
+  auto freeDecl = moduleBuilder.create<mlir::LLVM::LLVMFuncOp>(
+      rewriter.getUnknownLoc(), freeName,
       mlir::LLVM::LLVMFunctionType::get(voidType,
                                         getLlvmPtrType(op.getContext()),
                                         /*isVarArg=*/false));
+  return mlir::SymbolRefAttr::get(freeDecl);
 }
 
 static unsigned getDimension(mlir::LLVM::LLVMArrayType ty) {
@@ -1280,9 +1288,8 @@ struct FreeMemOpConversion : public FIROpConversion<fir::FreeMemOp> {
   mlir::LogicalResult
   matchAndRewrite(fir::FreeMemOp freemem, OpAdaptor adaptor,
                   mlir::ConversionPatternRewriter &rewriter) const override {
-    mlir::LLVM::LLVMFuncOp freeFunc = getFree(freemem, rewriter);
     mlir::Location loc = freemem.getLoc();
-    freemem->setAttr("callee", mlir::SymbolRefAttr::get(freeFunc));
+    freemem->setAttr("callee", getFree(freemem, rewriter));
     rewriter.create<mlir::LLVM::CallOp>(loc, mlir::TypeRange{},
                                         mlir::ValueRange{adaptor.getHeapref()},
                                         freemem->getAttrs());
diff --git a/flang/test/Fir/already-defined-free.fir b/flang/test/Fir/already-defined-free.fir
new file mode 100644
index 000000000000000..1a6aa7b5b5404bd
--- /dev/null
+++ b/flang/test/Fir/already-defined-free.fir
@@ -0,0 +1,22 @@
+// Test that FIR codegen handles cases when free and malloc have
+// already been defined in FIR (either by the user in Fortran via
+// BIND(C) or by some FIR pass in between).
+// RUN: fir-opt --fir-to-llvm-ir %s | FileCheck %s
+
+
+func.func @already_declared_free_malloc() {
+  %c4 = arith.constant 4 : index
+  %0 = fir.call @malloc(%c4) : (index) -> !fir.heap<i32>
+  fir.call @free(%0) : (!fir.heap<i32>) -> ()
+  %1 = fir.allocmem i32
+  fir.freemem %1 : !fir.heap<i32>
+  return
+}
+
+// CHECK: llvm.call @malloc(%{{.*}})
+// CHECK: llvm.call @free(%{{.*}})
+// CHECK: llvm.call @malloc(%{{.*}})
+// CHECK: llvm.call @free(%{{.*}})
+
+func.func private @free(!fir.heap<i32>)
+func.func private @malloc(index) -> !fir.heap<i32>

[vzakhari]

LGTM, though, this will still not work in case of function signature mismatch, e.g. if akin to #61074 (comment) free or malloc is defined by user code with a signature different from the standard one. We should probably report a reasonable error in such cases, but not in this PR.

[jeanPerier]

LGTM, though, this will still not work in case of function signature mismatch, e.g. if akin to #61074 (comment) free or malloc is defined by user code with a signature different from the standard one. We should probably report a reasonable error in such cases, but not in this PR.

Right, with this patch this will lead to MLIR errors looking like:

error: loc("repro.f90":12:3): 'llvm.call' op incorrect number of operands (1) for callee (expecting: 0)
error: loc("repro.f90":14:3): 'llvm.call' op operand type mismatch for operand 0: '!llvm.ptr' != 'i32'

Since the line number may point on allocate/deallocate statement rather than the conflicting free/malloc user def, I agree this is not a very useful hint regarding what is wrong.