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