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

[Alexis Perry-Holby via Phabricator via flang-commits]

AlexisPerry added a comment.

Thanks so much for the review!  I've been a little preoccupied of late, so this patch wasn't quite as polished as I would have like.  Thanks for helping me get it in better shape!



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:76
+  /// appropriate reified structures.
+  mlir::Value integerCast(mlir::Location loc,
+                          mlir::ConversionPatternRewriter &rewriter,
----------------
clementval wrote:
> This function is just moved. Can you put it back at the same position so there is no change?
When I rebased on main I ended up with two of these same declarations in the file and apparently I chose the wrong one to remove.  Thanks for catching that!


================
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 {
----------------
clementval wrote:
> Is this change needed?
I couldn't build the code without this change, so I would say yes?


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


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:800
+
+namespace {
+/// convert to `call` to the runtime to `malloc` memory
----------------
clementval wrote:
> Can you remove the extra anonymous namespace? I think it's already in one. 
Whoops!  Thanks for the catch (again)!


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:809
+    auto ty = convertType(heap.getType());
+    auto mallocFunc = getMalloc(heap, rewriter);
+    auto loc = heap.getLoc();
----------------
clementval wrote:
> To make `auto` people happy :-)
Thank you for this.  Sometimes it can be hard for me to track down the types to replace auto with.


================
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:
> clementval wrote:
> > `CHECK-LABEL`?
> Can you move the check block after the MLIR code so it looks like other tests in the file?
Certainly.  I was patterning after the llvm.unreachable example, but I have taken the liberty to fix that test so that it matches this style as well.


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

https://reviews.llvm.org/D114104