[PATCH] D140415: [flang] stack arrays pass

[Tom Eccles via Phabricator via cfe-commits]

tblah added a comment.

In D140415#4098170 <https://reviews.llvm.org/D140415#4098170>, @kiranchandramohan wrote:

> 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
>     }
>   }

Yes we could have used Dominance and PostDominance information to find out if an allocation is always freed. I wasn't aware of `mlir::DominanceInfo` at the time I wrote this patch. As It is already written, I think the data flow analysis continues to be the correct approach because it will skip dead code (after constant propagation) and I suspect the worst case algorithmic complexity is better than computing dominance between each heap allocation and free.

Yes in that case we cannot detect that the allocation is freed because the free operates on a different SSA value to the allocations. This would have been a problem whether `mlir::DominanceInfo` or `mlir::DataFlowAnalysis` were used. I chose not to support allocations and frees using different SSA values as this would have added considerable complexity and is not necessary for the more common cases of Flang-generated allocations. See the RFC for details.



================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:532
+
+  // Find when the last operand value becomes available
+  mlir::Block *operandsBlock = nullptr;
----------------
kiranchandramohan wrote:
> Might be worth checking whether we have a function for this in MLIR core.
Not that I can find. The MLIR verifier checks that all operation arguments properly dominate the operation, but this is done by comparing each in turn against the operation: no last operand is found.

I could use mlir::DominanceInfo to find when the last operand becomes available, which I guess would better handle the case where operands are defined in different blocks. But dominance only provides a partial ordering so there might be cases where `domInfo.properlyDominates(arg1, arg2) == domInfo.properlyDominates(arg2, arg1) == false`. Looking at the direct operation ordering only within the same block (as I do here) guarantees a total ordering relationship.


================
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());
----------------
kiranchandramohan wrote:
> 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)
> ```
> 
Thank you for pointing out `mlir::DominanceInfo` - I was not aware of that analysis. I propose we keep this pass as it is for now, to avoid adding more complexity where we don't have a concrete example of flang-generated allocations which need to support alloca arguments defined in different blocks.


================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:560
+        lastOperand->getParentOfType<mlir::omp::OutlineableOpenMPOpInterface>();
+    if (lastOpOmpRegion == oldOmpRegion)
+      return checkReturn(lastOperand);
----------------
kiranchandramohan wrote:
> Do we have a test for this, and in general for the OpenMP handling?
When writing the tests I discovered that the data flow analysis does not propagate lattices into or out of an omp.section, so currently no allocations inside of an openmp secton will be moved to the stack.

I intend to handle this in a subsequent patch. In the meantime I have added a test to make sure that allocations in an openmp region are not moved.


================
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();
----------------
kiranchandramohan wrote:
> Nit: Is this error usually given in passes?
Sorry I don't understand. What change are you requesting here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140415