[Mlir-commits] [flang] [mlir] [mlir][CSE] Add pruneDeadOps to CSE pass (PR #193778)

[lonely eagle]

================
@@ -294,15 +297,38 @@ LogicalResult CSEDriver::simplifyOperation(ScopedMapTy &knownValues,
   return failure();
 }
 
+void CSEDriver::pruneDeadOps(Operation *root, ScopedMapTy &knownValues) {
+  // We use `SetVector` to prevent already inserted ops from being added to the
+  // `worklist` repeatedly, avoiding secondary access to erased operations.
+  llvm::SetVector<Operation *> worklist;
----------------
linuxlonelyeagle wrote:

> Right. Which makes it unclear to me whether this belongs to CSE or should just be on the user to decide what to do before CSE (run trivial-dce or run canonicalize before calling CSE).

Honestly, I’m not sure why the original implementation of CSE included DCE in the first place either. However, it is true that prior DCE can expose further opportunities for CSE. Furthermore, the DCE process can be performed concurrently with the CSE traversal.

> One interesting question: is there a case where a DCE opportunity only presents itself after CSE merged some operations?

I have considered this scenario: 
* first
if `existingOp` and `op` are the same and both qualify as dead code, CSE's traversal order would result in `existingOp` being removed first, followed by the removal of `op` when it is visited, thus bypassing the CSE optimization entirely. 
* second
Alternatively, we can view it this way: once `op `is replaced by `existingOp`, `op` becomes redundant (dead code). But if the results of `existingOp` are consumed by other operations, then only `op` should be eliminated.
You might be thinking that since `op `is dead, the def-op of its operands would also become dead. But that shouldn't be the case, because `existingOp` and `op` share the same operands.
In my perspective, CSE functions like a merge operation that detaches the `op ` from its def-use chain. Whether the `existingOp` itself is eligible for deletion should be determined by the downstream branches. Therefore, I don't believe such a case would exist.

> Should be addressed by what I mentioned above:

Yes. I haven't given this much thought yet.

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