[PATCH] D140415: [flang] stack arrays pass

[Tom Eccles via Phabricator via cfe-commits]

tblah added inline comments.


================
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);
----------------
kiranchandramohan wrote:
> Do we have a test with multiple returns?
Thanks for this. It turned out I needed to join across all of the lattices at the return statements to ensure that values were returned at *all* return statements, not at *any* return statement.


================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:696-697
+
+void StackArraysPass::runOnOperation() {
+  mlir::ModuleOp mod = getOperation();
+
----------------
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.


================
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
----------------
kiranchandramohan wrote:
> Is this a case for future improvement?
Yes. This is an open TODO. I'll add a comment.

It should be possible to still do stack save/restore if the block containing the free is *always* executed after the memalloc. This might already be guaranteed by the data-flow analysis - I haven't thought enough about it. I haven't seen this happen in the allocations automatically generated by flang, so I don't think it is important to solve now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140415