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
