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

[Mehdi Amini via flang-commits]

================
@@ -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;
+  worklist.insert(root);
+  while (!worklist.empty()) {
+    Operation *op = worklist.front();
+    worklist.erase(worklist.begin());
+    if (!isOpTriviallyDead(op))
+      continue;
+
+    for (Value arg : op->getOperands())
+      if (Operation *argOp = arg.getDefiningOp())
+        worklist.insert(argOp);
+
+    // Since the root op is not inserted into the ScopedHashMap, do not undo
+    // its previous insertion.
----------------
joker-eph wrote:

I have quite a few concerns with this patch, in no particular order:

- Efficiency: the implementation isn't the most optimal. And there is no complexity improvement over running DCE before/after CSE.
- Code duplication: this is an algorithm that does not belong to CSE.
- Usefulness: I don't think I saw an example showing this is needed when we have the ability to run DCE before/after if needed.
- Correctness: the public API can run CSE on a given region, looks like you would delete operations outside of this region. This is breaking the API contract IMO.


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