[Mlir-commits] [mlir] [MLIR][Mem2Reg] Add support for region control flow and SCF (PR #185036)

[Tobias Gysi]

================
@@ -430,13 +480,30 @@ MemorySlotPromotionAnalyzer::computeInfo() {
   // promotion to happen. These operations need to resolve some of their uses,
   // either by rewiring them or simply deleting themselves. If any of them
   // cannot find a way to resolve their blocking uses, we abort the promotion.
-  if (failed(computeBlockingUses(info.userToBlockingUses)))
+  // We also compute at this stage the regions that will be analyzed for
+  // reaching definition information.
+  if (failed(
+          computeBlockingUses(info.userToBlockingUses, info.regionsToPromote)))
     return {};
 
+  // Compute the blocks containing a store for each region, either directly or
+  // inherited from a nested region. As a side effect, `definingBlocks` contains
+  // all regions with at least one store.
+  DenseMap<Region *, SmallPtrSet<Block *, 16>> definingBlocks;
+  for (Operation *user : slot.ptr.getUsers())
+    if (auto storeOp = dyn_cast<PromotableMemOpInterface>(user))
+      if (storeOp.storesTo(slot))
+        definingBlocks[user->getParentRegion()].insert(user->getBlock());
+  for (auto &[region, regionInfo] : info.regionsToPromote)
+    if (regionInfo.hasValueStores)
+      definingBlocks[region->getParentRegion()].insert(
+          region->getParentOp()->getBlock());
----------------
gysit wrote:

But why is it sufficient to only add the direct ancestor here and not all ancestors transitively? Maybe this is happening somehow but it is at least not obvious to me. You can also convince me by having a deeply nested test with multiple adjacent levels that don't contain a write / read.

https://github.com/llvm/llvm-project/pull/185036