[PATCH] D140415: [flang] stack arrays pass

[Jean Perier via Phabricator via cfe-commits]

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