[PATCH] D140415: [flang] stack arrays pass

[Kiran Chandramohan via Phabricator via cfe-commits]

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