[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:120
+  // which operations we intend to rewrite before it calls the pattern rewriter
+  llvm::SmallDenseMap<mlir::Value, InsertionPoint, 1> insertionPoints;
+
----------------
jeanPerier wrote:
> It is definitely weird to me to have this in the lattice points. It seems expensive to save this at every program point and to compute it every time a state changes on a maybe not final candiate.
> 
> Why not computing in StackArraysAnalysisWrapper::analyseFunction on the final candidates (the value in stateMap at that are freed on all return paths) ?
Good idea. Thanks!


================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:460
+      lattice->appendFreedValues(candidateOps);
+    }
+  });
----------------
jeanPerier wrote:
> I think this is not correct: It seems this will consider every FreememOp that could be paired with an allocmem as candidate:
> 
> ```
> func.func @test(%cdt: i1) {
>   %a = fir.allocmem !fir.array<1xi8>
>   scf.if %cdt {
>     fir.freemem %a : !fir.heap<!fir.array<1xi8>>
>   } else {
>   }
>   return
> }
> ```
> 
> Why not considering the func.return lattice states instead ?
> 
> Note that it seems fir.if is not considered a branch operation, so the state of its blocks are reset on entry. That is why scf.if is needed to create a test exposing the issue. Maybe fir.if op should be given the right interface, but that is another topic.
Good spot! To get analysis working with this change I've had to add processing of fir.result operations. These will join the parent operation's lattice with the fir.result.


================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:509
+
+static bool isInLoop(mlir::Block *block) { return mlir::blockIsInLoop(block); }
+
----------------
jeanPerier wrote:
> Where is `blockIsInLoop` defined ?
https://reviews.llvm.org/D141401


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140415