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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
