[flang-commits] [PATCH] D114104: [FIR] Convert fir.allocmem and fir.freemem operations to calls to malloc and free, respectively

[Andrzej Warzynski via Phabricator via flang-commits]

awarzynski added a comment.

Thanks for working on this @AlexisPerry!

> would really appreciate it if you could point me to a good example to pattern after.

Sure! Looking at the code from your patch I am guessing that you'd need to test it with a record type (i.e. fir.type <https://github.com/llvm/llvm-project/blob/b57e2f071a2e47147a57c52a6a8c6aa062230cd8/flang/include/flang/Optimizer/Dialect/FIRTypes.td#L287-L325>) with at least one LEN parameter:

  if (auto recTy =
           fir::unwrapSequenceType(heap.getType()).dyn_cast<fir::RecordType>())
     if (recTy.getNumLenParams() != 0)
       return rewriter.notifyMatchFailure(
           loc, "fir.allocmem of derived type with length parameters");

Here's an example <https://github.com/llvm/llvm-project/blob/b57e2f071a2e47147a57c52a6a8c6aa062230cd8/flang/test/Fir/convert-to-llvm.fir#L987-L1000> of such type. You can probably simplify it a bit. Once you have an example that hits `notifyMatchFailure` in `AllocMemOpConversion`, you can use it to create a test in convert-to-llvm-invalid.fir <https://github.com/llvm/llvm-project/blob/b57e2f071a2e47147a57c52a6a8c6aa062230cd8/flang/test/Fir/convert-to-llvm-invalid.fir>.



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:874
+      op->getParentOfType<mlir::ModuleOp>().getBodyRegion());
+  auto indexType = mlir::IntegerType::get(op.getContext(), 64);
+  return moduleBuilder.create<mlir::LLVM::LLVMFuncOp>(
----------------
I feel that we should use [[ https://github.com/llvm/llvm-project/blob/main/flang/lib/Optimizer/CodeGen/TypeConverter.h#L122 | indexType() ]] here instead (otherwise this duplicates that method). This would probably require turning this into a member method though. Alternatively, the result of `indexType()` could be one of the parameters.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:892
+    mlir::LLVM::LLVMFuncOp mallocFunc = getMalloc(heap, rewriter);
+    auto loc = heap.getLoc();
+    auto ity = lowerTy().indexType();
----------------



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:901
+    for (mlir::Value opnd : adaptor.getOperands())
+      size = rewriter.create<mlir::LLVM::MulOp>(
+          loc, ity, size, integerCast(loc, rewriter, ity, opnd));
----------------
Could you add a test that triggers this code-path? I'd try with multi-dim arrays.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:943
+namespace {
+/// lower a freemem instruction into a call to free()
+struct FreeMemOpConversion : public FIROpConversion<fir::FreeMemOp> {
----------------
This is a nit. If you agree with this suggestion,  could  you also update other function comments?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:951
+    mlir::LLVM::LLVMFuncOp freeFunc = getFree(freemem, rewriter);
+    auto loc = freemem.getLoc();
+    auto bitcast = rewriter.create<mlir::LLVM::BitcastOp>(
----------------



================
Comment at: flang/test/Fir/convert-to-llvm.fir:197
+// CHECK: [[NULL:%.*]]  = llvm.mlir.null : !llvm.ptr<array<100 x f32>> 
+// CHECK: [[ONE:%.*]]  = llvm.mlir.constant(1 : i64) : i64 
+// CHECK: [[PTR:%.*]]  = llvm.getelementptr [[NULL]][{{.*}}] : (!llvm.ptr<array<100 x f32>>, i64) -> !llvm.ptr<array<100 x f32>> 
----------------
What's `ONE` for? If it's not used then perhaps it should not be generated?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114104/new/

https://reviews.llvm.org/D114104