kiranchandramohan added a comment. Thanks for this patch @tblah. I had a first look. See comments inline. Have not gone through the core part in full yet.
================ Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:433 + llvm::DenseSet<mlir::Value> freedValues; + func->walk([&](mlir::func::ReturnOp child) { + const LatticePoint *lattice = solver.lookupState<LatticePoint>(child); ---------------- Do we have a test with multiple returns? ================ Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:535 + LLVM_DEBUG(llvm::dbgs() << "--considering operand " << operand << "\n"); + mlir::Operation *op = operand.getDefiningOp(); + if (!operandsBlock) ---------------- The op might require a check before further use. See the following test from `arrexp.fir`. (run with `./bin/tco f4.fir`) ``` func.func @f4(%a : !fir.ref<!fir.array<?x?xf32>>, %b : !fir.ref<!fir.array<?x?xf32>>, %n : index, %m : index, %o : index, %p : index, %f : f32) { %c1 = arith.constant 1 : index %s = fir.shape_shift %o, %n, %p, %m : (index, index, index, index) -> !fir.shapeshift<2> %vIn = fir.array_load %a(%s) : (!fir.ref<!fir.array<?x?xf32>>, !fir.shapeshift<2>) -> !fir.array<?x?xf32> %wIn = fir.array_load %b(%s) : (!fir.ref<!fir.array<?x?xf32>>, !fir.shapeshift<2>) -> !fir.array<?x?xf32> %r = fir.do_loop %j = %p to %m step %c1 iter_args(%v1 = %vIn) -> !fir.array<?x?xf32> { %r = fir.do_loop %i = %o to %n step %c1 iter_args(%v = %v1) -> !fir.array<?x?xf32> { %x2 = fir.array_fetch %vIn, %i, %j : (!fir.array<?x?xf32>, index, index) -> f32 %x = fir.array_fetch %wIn, %i, %j : (!fir.array<?x?xf32>, index, index) -> f32 %y = arith.addf %x, %f : f32 %y2 = arith.addf %y, %x2 : f32 %i2 = arith.addi %i, %c1 : index %r = fir.array_update %v, %y2, %i2, %j : (!fir.array<?x?xf32>, f32, index, index) -> !fir.array<?x?xf32> fir.result %r : !fir.array<?x?xf32> } fir.result %r : !fir.array<?x?xf32> } fir.array_merge_store %vIn, %r to %a : !fir.array<?x?xf32>, !fir.array<?x?xf32>, !fir.ref<!fir.array<?x?xf32>> return } ``` ================ Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:620-622 + if (it == candidateOps.end()) { + return {}; + } ---------------- Nit: Braces not required. ================ Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:630-638 + mlir::Location loc = oldAlloc.getLoc(); + mlir::Type varTy = oldAlloc.getInType(); + auto unpackName = [](std::optional<llvm::StringRef> opt) -> llvm::StringRef { + if (opt) + return *opt; + return {}; + }; ---------------- Nit: Move all these close to the creation of the `fir:AllocaOp`. ================ Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:640-641 + + if (mlir::Operation *op = insertionPoint.tryGetOperation()) + rewriter.setInsertionPointAfter(op); + else { ---------------- kiranchandramohan wrote: > Nit: Use braces here to match `else`. Nit: Use braces for the `if` block to keep it uniform with the `else` block. ================ Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:640-642 + if (mlir::Operation *op = insertionPoint.tryGetOperation()) + rewriter.setInsertionPointAfter(op); + else { ---------------- Nit: Use braces here to match `else`. ================ Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:696-697 + +void StackArraysPass::runOnOperation() { + mlir::ModuleOp mod = getOperation(); + ---------------- From the following code, it seems the functions are processed independently. Can this be a `Function` pass? ================ Comment at: flang/test/Transforms/stack-arrays.f90:136 +! CHECK-SAME: cfgloop +! CHECK-NEXT: %0 = fir.alloca !fir.array<100000000xi32> +! CHECK-NOT: fir.allocmem ---------------- Nit: Remove usage of `%0`. ================ Comment at: flang/test/Transforms/stack-arrays.fir:39-49 +// CHECK: func.func @dfa1(%arg0: !fir.ref<!fir.logical<4>> {fir.bindc_name = "cond"}) { +// CHECK-NEXT: %c42 = arith.constant 42 : index +// CHECK-NEXT: %0 = fir.allocmem !fir.array<?xi32>, %c42 {uniq_name = "_QFdfa1Earr.alloc"} +// CHECK-NEXT: %1 = fir.load %arg0 : !fir.ref<!fir.logical<4>> +// CHECK-NEXT: %2 = fir.convert %1 : (!fir.logical<4>) -> i1 +// CHECK-NEXT: fir.if %2 { +// CHECK-NEXT: fir.freemem %0 : !fir.heap<!fir.array<?xi32>> ---------------- Would it be better to capture the variables and check? At least the allocmem and freemem. ================ Comment at: flang/test/Transforms/stack-arrays.fir:203 + +// check that stack save/restore are not used when the memalloc and freemem are +// in different blocks ---------------- Is this a case for future improvement? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140415/new/ https://reviews.llvm.org/D140415 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits