[PATCH] D140415: [flang] stack arrays pass

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Looks OK. I have a few questions and some minor comments inline. It might be good to pull in a bit of info from the RFC into the Summary, particularly saying why a dataflow analysis is necessary, what operations are involved in the analysis etc.

Could we have used the Dominance and PostDominance information to find out the Allocs and Frees that could have been replaced? I saw the following functions for individual Ops but not for the case where a set of ops dominates or post-dominates. So may be not with the existing infra.

  bool DominanceInfo::properlyDominatesImpl(Operation *a, Operation *b
  bool PostDominanceInfo::properlyPostDominates(Operation *a, Operation *b) {

I guess, we are not capturing the following because of different values.

  module {
    func.func @dfa2(%arg0: i1) {
      cf.cond_br %arg0, ^bb1, ^bb2
    ^bb1:  // pred: ^bb0
      %a = fir.allocmem !fir.array<1xi8>
      cf.br ^bb3(%a : !fir.heap<!fir.array<1xi8>>)
    ^bb2:  // pred: ^bb0
      %b = fir.allocmem !fir.array<1xi8>
      cf.br ^bb3(%b : !fir.heap<!fir.array<1xi8>>)
    ^bb3(%0: !fir.heap<!fir.array<1xi8>>):  // 2 preds: ^bb1, ^bb2
      fir.freemem %0 : !fir.heap<!fir.array<1xi8>>
      return
    }
  }



================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:436-438
+    if (lattice) {
+      point.join(*lattice);
+    }
----------------
Nit: No brace here


================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:442
+  point.appendFreedValues(freedValues);
+
+  for (mlir::Value freedValue : freedValues) {
----------------
A comment here would be useful on why we need to look at the freed values only.


================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:482-486
+  for (mlir::Operation *user : allocmem.getOperation()->getUsers()) {
+    if (mlir::isa<fir::FreeMemOp>(user)) {
+      rewriter.eraseOp(user);
+    }
+  }
----------------
Nit: Braces might not be require here.


================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:532
+
+  // Find when the last operand value becomes available
+  mlir::Block *operandsBlock = nullptr;
----------------
Might be worth checking whether we have a function for this in MLIR core.


================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:545-547
+      // Operation::isBeforeInBlock requires the operations to be in the same
+      // block. The best we can do is the location of the allocmem.
+      return checkReturn(oldAlloc.getOperation());
----------------
Theoretically speaking, we can use the dominance info to determine whether one block dominates the other as well to handle cases like the following where we are finding the operands of `func`. But I guess that is probably not required.
```
b1:
x = opA
br b2
b2:
y = opB
br b3
b3:
z = func(x,y)
```



================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:560
+        lastOperand->getParentOfType<mlir::omp::OutlineableOpenMPOpInterface>();
+    if (lastOpOmpRegion == oldOmpRegion)
+      return checkReturn(lastOperand);
----------------
Do we have a test for this, and in general for the OpenMP handling?


================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:572-574
+  if (oldOmpRegion) {
+    return checkReturn(oldOmpRegion.getAllocaBlock());
+  }
----------------
Nit: No need for braces here.


================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:593-597
+  for (mlir::Operation *user : oldAllocOp->getUsers()) {
+    if (mlir::isa<fir::FreeMemOp>(user)) {
+      freeOps.push_back(user);
+    }
+  }
----------------
Nit: braces are not required here.


================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:732
+          mlir::applyPartialConversion(func, target, std::move(patterns)))) {
+    mlir::emitError(func->getLoc(), "error in stack arrays optimization\n");
+    signalPassFailure();
----------------
Nit: Is this error usually given in passes?


================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:696-697
+
+void StackArraysPass::runOnOperation() {
+  mlir::ModuleOp mod = getOperation();
+
----------------
tblah wrote:
> kiranchandramohan wrote:
> > From the following code, it seems the functions are processed independently. Can this be a `Function` pass?
> It can't. `fir::factory::getLlvm::getStackSave` and `fir::factory::getLlvmSatckRestore` add function declarations to the module-level. If functions are processed in different threads, there is a race condition when the `fir::builder` first checks to see if the function already exists in the module and if not, adds it.
Not for this patch: May be these can all be preinserted at the beginning of the pass pipeline and removed if not used at the end of the pass pipeline?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140415