[Mlir-commits] [mlir] [MLIR] Prevent invalid IR from being passed outside of RemoveDeadValues (PR #121079)

[Andrzej Warzyński]

================
@@ -264,51 +321,62 @@ static void cleanFuncOp(FunctionOpInterface funcOp, Operation *module,
   for (Block &block : funcOp.getBlocks()) {
     Operation *returnOp = block.getTerminator();
     if (returnOp && returnOp->getNumOperands() == numReturns)
-      returnOp->eraseOperands(nonLiveRets);
+      cl.operands.push_back({returnOp, nonLiveRets});
   }
-  funcOp.eraseResults(nonLiveRets);
+  cl.functions.push_back({funcOp, nonLiveArgs, nonLiveRets});
 
   // Do (5) and (6).
   for (SymbolTable::SymbolUse use : uses) {
     Operation *callOp = use.getUser();
     assert(isa<CallOpInterface>(callOp) && "expected a call-like user");
-    dropUsesAndEraseResults(callOp, nonLiveRets);
+    cl.results.push_back({callOp, nonLiveRets});
+    collectNonLiveValues(deletionSet, callOp->getResults(), nonLiveRets);
   }
 }
 
-/// Clean a region branch op `regionBranchOp`, given the liveness information in
-/// `la`. Here, cleaning means:
-///   (1') Dropping all its uses, AND
-///   (2') Erasing it
-/// if it has no memory effects and none of its results are live, AND
-///   (1) Erasing its unnecessary operands (operands that are forwarded to
-///   unneccesary results and arguments),
-///   (2) Cleaning each of its regions,
-///   (3) Dropping the uses of its unnecessary results (results that are
-///   forwarded from unnecessary operands and terminator operands), AND
-///   (4) Erasing these results
-/// otherwise.
-/// Note that here, cleaning a region means:
-///   (2.a) Dropping the uses of its unnecessary arguments (arguments that are
-///   forwarded from unneccesary operands and terminator operands),
-///   (2.b) Erasing these arguments, AND
-///   (2.c) Erasing its unnecessary terminator operands (terminator operands
-///   that are forwarded to unneccesary results and arguments).
-/// It is important to note that values in this op flow from operands and
-/// terminator operands (successor operands) to arguments and results (successor
-/// inputs).
-static void cleanRegionBranchOp(RegionBranchOpInterface regionBranchOp,
-                                RunLivenessAnalysis &la) {
+/// Process a region branch operation `regionBranchOp` using the liveness
+/// information in `la`. The processing involves two scenarios:
+///
+/// Scenario 1: If the operation has no memory effects and none of its results
+/// are live:
+///   1. Enqueue all its uses for deletion.
+///   2. Enqueue the branch itself for deletion.
+///
+/// Scenario 2: Otherwise:
+///   1. Collect its unnecessary operands (operands forwarded to unnecessary
+///   results or arguments).
+///   2. Process each of its regions.
+///   3. Collect the uses of its unnecessary results (results forwarded from
+///   unnecessary operands
+///      or terminator operands).
+///   4. Add these results to the deletion list.
+///
+/// Processing a region includes:
+///   a. Collecting the uses of its unnecessary arguments (arguments forwarded
+///   from unnecessary operands
+///      or terminator operands).
+///   b. Collecting these unnecessary arguments.
+///   c. Collecting its unnecessary terminator operands (terminator operands
+///   forwarded to unnecessary results
+///      or arguments).
+///
+/// Value Flow Note: In this operation, values flow as follows:
+/// - From operands and terminator operands (successor operands)
+/// - To arguments and results (successor inputs).
----------------
banach-space wrote:

I like how you split it into Scenario 1 and Scenario 2, thanks!

I suggest preserving the original format for bullet points (`(1)` instead of `1.`), so that every step can be clearly referred to in the implementation (that's how it's done currently).

Also, I'm wondering how to clearly split steps for Scenario 1 and Scenario 2. How about:
* (S1 1) -> (S1 2) for steps 1 and 2 for Scenario 1, and
* (S2 1) -> (S2 2) -> (S2 3) ->  ... for Scenario 2?

On second thought, given how basic Scenario 1 is, perhaps we only need (1) -> (2) -> (3) for Scenario 2? Either way, it would be good for the steps in comments to match the steps in the code.

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