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

[Valentin Clement via Phabricator via flang-commits]

clementval added a comment.

Thanks for working on this Alexis. Just a couple of comments before we can proceed.



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:76
+  /// appropriate reified structures.
+  mlir::Value integerCast(mlir::Location loc,
+                          mlir::ConversionPatternRewriter &rewriter,
----------------
This function is just moved. Can you put it back at the same position so there is no change?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:223
   mlir::LogicalResult
-  matchAndRewrite(fir::AbsentOp absent, OpAdaptor,
+  matchAndRewrite(fir::AbsentOp absent, OpAdaptor adaptor,
                   mlir::ConversionPatternRewriter &rewriter) const override {
----------------
Is this change needed?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:245
 
+namespace {
+/// Helper function for generating the LLVM IR that computes the size
----------------
I think it's already in an anonymous namespace.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:800
+
+namespace {
+/// convert to `call` to the runtime to `malloc` memory
----------------
Can you remove the extra anonymous namespace? I think it's already in one. 


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:809
+    auto ty = convertType(heap.getType());
+    auto mallocFunc = getMalloc(heap, rewriter);
+    auto loc = heap.getLoc();
----------------
To make `auto` people happy :-)


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:816
+        TODO(loc, "fir.allocmem of derived type with length parameters");
+    auto size = genTypeSizeInBytes(loc, ity, rewriter, ty);
+    for (auto opnd : adaptor.getOperands())
----------------



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:817
+    auto size = genTypeSizeInBytes(loc, ity, rewriter, ty);
+    for (auto opnd : adaptor.getOperands())
+      size = rewriter.create<mlir::LLVM::MulOp>(
----------------



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:859
+
+namespace {
+/// lower a freemem instruction into a call to free()
----------------
Same comment for the anonymous namespace


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:867
+                  mlir::ConversionPatternRewriter &rewriter) const override {
+    auto freeFunc = getFree(freemem, rewriter);
+    auto loc = freemem.getLoc();
----------------
To make `auto` people happy :-)


================
Comment at: flang/test/Fir/convert-to-llvm.fir:172
+
+// CHECK:  llvm.func @test_alloc_and_freemem_one() {
+// CHECK-NEXT:    [[N:%.*]] = llvm.mlir.constant(4 : i64) : i64
----------------
`CHECK-LABEL`?


================
Comment at: flang/test/Fir/convert-to-llvm.fir:172-176
+// CHECK:  llvm.func @test_alloc_and_freemem_one() {
+// CHECK-NEXT:    [[N:%.*]] = llvm.mlir.constant(4 : i64) : i64
+// CHECK-NEXT:    llvm.call @malloc([[N]])
+// CHECK:         llvm.call @free(%{{.*}})
+// CHECK-NEXT:    llvm.return
----------------
clementval wrote:
> `CHECK-LABEL`?
Can you move the check block after the MLIR code so it looks like other tests in the file?


================
Comment at: flang/test/Fir/convert-to-llvm.fir:189-198
+// CHECK:  llvm.func @test_alloc_and_freemem_several() {
+// 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>> 
+// CHECK: [[N:%.*]]  = llvm.ptrtoint [[PTR]] : !llvm.ptr<array<100 x f32>> to i64
+// CHECK: [[MALLOC:%.*]] = llvm.call @malloc([[N]])
+// CHECK: [[B1:%.*]] = llvm.bitcast [[MALLOC]] : !llvm.ptr<i8> to !llvm.ptr<array<100 x f32>>
----------------
CHECK-LABEL and move the block after the MLIR code. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114104