[PATCH] D77714: Extended Liveness analysis to support nested regions.

[River Riddle via Phabricator via llvm-commits]

rriddle requested changes to this revision.
rriddle added a comment.
This revision now requires changes to proceed.

Thanks for tackling this! Left a few comments.



================
Comment at: mlir/lib/Analysis/Liveness.cpp:64
+      // additional conditions to detect an escaping value.
+      for (OpOperand &use : value.getUses()) {
+        Block *ownerBlock = use.getOwner()->getBlock();
----------------
nit: Use `getUsers` if you are only accessing the use owners.


================
Comment at: mlir/lib/Analysis/Liveness.cpp:69
+        // is used in a nested region.
+        ownerBlock = findInCurrent(
+            block->getParent(), ownerBlock,
----------------
Can you just add a utility `Region::findAncestorOpInRegion(Operation *op)` method that finds the ancestor of an operation within a given region? It's effectively similar to the block variant.


================
Comment at: mlir/lib/Analysis/Liveness.cpp:423
   Operation *endOperation = startOperation;
   for (OpOperand &use : value.getUses()) {
     Operation *useOperation = use.getOwner();
----------------
nit: Same here, can you change this to `getUsers`?


================
Comment at: mlir/lib/Analysis/Liveness.cpp:426
+    // Find the associated operation in the current block (if any).
+    useOperation = findInCurrent(
+        block, useOperation, [](Operation *op) { return op->getBlock(); },
----------------
Are you trying to find an ancestor of useOperation that exists within block? Seems like `block->findAncestorOpInBlock(useOperation)` is intended for this use case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77714