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

[Andrzej Warzyński]

================
@@ -165,52 +214,59 @@ static SmallVector<OpOperand *> operandsToOpOperands(OperandRange operands) {
   return opOperands;
 }
 
-/// Clean a simple op `op`, given the liveness analysis information in `la`.
-/// Here, cleaning means:
-///   (1) Dropping all its uses, AND
-///   (2) Erasing it
-/// iff it has no memory effects and none of its results are live.
+/// Process a simple operation `op` using the liveness analysis `la`.
+/// If the operation has no memory effects and none of its results are live:
+///   1. Adding the operation to a list for future removal, and
+///   2. Marking all its results as non-live values
 ///
-/// It is assumed that `op` is simple. Here, a simple op is one which isn't a
-/// function-like op, a call-like op, a region branch op, a branch op, a region
-/// branch terminator op, or return-like.
-static void cleanSimpleOp(Operation *op, RunLivenessAnalysis &la) {
-  if (!isMemoryEffectFree(op) || hasLive(op->getResults(), la))
+/// The operation `op` is assumed to be simple.
+/// A simple operation is one that is NOT:
+///   - Function-like
+///   - Call-like
+///   - A region branch operation
+///   - A branch operation
+///   - A region branch terminator
+///   - Return-like
+static void processSimpleOp(Operation *op, RunLivenessAnalysis &la,
+                            DenseSet<Value> &deletionSet,
+                            RDVFinalCleanupList &cl) {
+  if (!isMemoryEffectFree(op) || hasLive(op->getResults(), deletionSet, la))
     return;
 
-  op->dropAllUses();
-  op->erase();
+  cl.operations.push_back(op);
+  collectNonLiveValues(deletionSet, op->getResults(),
+                       BitVector(op->getNumResults(), true));
 }
 
-/// Clean a function-like op `funcOp`, given the liveness information in `la`
-/// and the IR in `module`. Here, cleaning means:
-///   (1) Dropping the uses of its unnecessary (non-live) arguments,
-///   (2) Erasing these arguments,
-///   (3) Erasing their corresponding operands from its callers,
-///   (4) Erasing its unnecessary terminator operands (return values that are
-///   non-live across all callers),
-///   (5) Dropping the uses of these return values from its callers, AND
-///   (6) Erasing these return values
-/// iff it is not public or external.
-static void cleanFuncOp(FunctionOpInterface funcOp, Operation *module,
-                        RunLivenessAnalysis &la) {
+/// Process a function-like operation `funcOp` using the liveness analysis `la`
+/// and the IR in `module`. If it is not public or external:
+///   1. Adding its non-live arguments to a list for future removal.
+///   2. Marking their corresponding operands in its callers for removal.
+///   3. Identifying and enqueueing unnecessary terminator operands
+///      (return values that are non-live across all callers) for removal.
+///   4. Enqueueing the non-live arguments themselves for removal.
+///   5. Collecting the uses of these return values in its callers for future
+///   removal.
+///   6. Marking all its results as non-live values.
----------------
banach-space wrote:

Apologies in advance, but this comment is quite important, so I will be pedantic.

1. Please preserve the original, consistent form of numbering within this comment and within the implementation (i.e. "(2)" and "(2)" rather than "2." and "(2)").
2. "iff it is not public or external" stands for "if and only if ...". IMO, it's important to keep "iff".  Also, I don't see much need to move that sentence.
3. Using gerund (i.e. "Adding") after "means" (as in, "means Adding") is fine. But without "means" it feels off :)
4. Where does (4) actually happen?

```suggestion
/// Process a function-like op `funcOp`, given the liveness information in `la`
/// and the IR in `module`. Here, processing means:
///   (1) Adding its non-live arguments to a list for future removal.
///   (2) Marking their corresponding operands in its callers for removal.
///   (3) Identifying and enqueueing unnecessary terminator operands
///      (return values that are non-live across all callers) for removal.
///   (4) Enqueueing the non-live arguments themselves for removal.
///   (5) Collecting the uses of these return values in its callers for future
///   removal.
///   6. Marking all its results as non-live values.
/// iff it is not public or external.
```

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