jeanPerier added inline comments.
Herald added a subscriber: sunshaoce.
================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:104
+
+ bool operator!=(const InsertionPoint &rhs) const {
+ return (location != rhs.location) ||
----------------
It's better to negate the `== operator` here so that the implementation logic
cannot diverge.
================
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;
+
----------------
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) ?
================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:272
+ // it gets freed by the time we return
+ return AllocationState::Allocated;
+ }
----------------
This is still odd to me because this breaks the monocity requirement of join:
`join(join(freed, unknown), allocated) ) = join(unknown, allocated) = allocated`
while
`join(freed, join(unknown, allocated)) = join(freed, allocated) = unknown`
I still do not think you need anything special here given the fact that an
allocation done on a path is considered in the end already seems accounted for
in LatticePoint::join since the state is added even if not present in the other
latice.
================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:333
+ stateMap[value] = state;
+ addInsertionPoint(value, state);
+ return mlir::ChangeResult::Change;
----------------
As mentioned in my other comment above, I do not get why the insertion point is
computed at that point while it seems the analysis (after computing the states,
and using the lattice state at the func.return) is not over for the function (I
would expect insertion to be computed only for the successfully identified
allocmem at the end, not the one that may be candidate on one code path).
================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:460
+ lattice->appendFreedValues(candidateOps);
+ }
+ });
----------------
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.
================
Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:509
+
+static bool isInLoop(mlir::Block *block) { return mlir::blockIsInLoop(block); }
+
----------------
Where is `blockIsInLoop` defined ?
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
