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

[Kiran Chandramohan via Phabricator via flang-commits]

kiranchandramohan added a comment.

A few nit comments.



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:805
+                  mlir::ConversionPatternRewriter &rewriter) const override {
+    auto ty = convertType(heap.getType());
+    mlir::LLVM::LLVMFuncOp mallocFunc = getMalloc(heap, rewriter);
----------------
nit: mlir::Type


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:812
+      if (recTy.getNumLenParams() != 0)
+        TODO(loc, "fir.allocmem of derived type with length parameters");
+    mlir::Value size = genTypeSizeInBytes(loc, ity, rewriter, ty);
----------------
Nit: We have been using notifyMatch failure instead of TODOs in upstream.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:864
+    auto loc = freemem.getLoc();
+    auto bitcast = rewriter.create<mlir::LLVM::BitcastOp>(
+        freemem.getLoc(), voidPtrTy(), adaptor.getOperands()[0]);
----------------
Not for this patch: If we can write a wrapper function that creates the bitcast if the types are different but does not if they are the same, it might save a bitcast. LLVM will anyway remove, so not very important.


================
Comment at: flang/test/Fir/convert-to-llvm.fir:214-217
+// CHECK:  llvm.func @test_unreachable() {
+// CHECK-NEXT:    llvm.unreachable
+// CHECK-NEXT:  }
+
----------------
Nit: Is there a change here? If not it might be good to keep it as is.


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

https://reviews.llvm.org/D114104