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

[lonely eagle 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.
----------------
linuxlonelyeagle wrote:

Sorry for the wait, I spent some time rebuilding the project. "If a dead op happens to be identical to an existing op, we could inadvertently miss out on CSE optimization opportunities". You can see this, 
I’ve written an example to verify it. I hope this provides a better understanding of the implementation😉, Here is the result after removing the if statement.
```
func.func @test(%arg0: i32) -> (i32, i32){
  %0 = arith.addi %arg0, %arg0 : i32
  arith.addi %arg0, %arg0 : i32 
  %1 = arith.addi %arg0, %arg0 : i32
  return %1, %0 : i32, i32
}
```
```
~ # mlir-opt a.mlir -cse
module {
  func.func @test(%arg0: i32) -> (i32, i32) {
    %0 = arith.addi %arg0, %arg0 : i32
    %1 = arith.addi %arg0, %arg0 : i32
    return %1, %0 : i32, i32
  }
}
```
> I don't think that's the case. In practice, CSE only performs a single insertion for any given variable. Therefore, even if you attempt to erase a dead operation multiple times, it won't cause any issues

This addresses your previous point about “That would be a concern for the other ops that will be deleted in the process too though.” It is unrelated to the concern that “If a dead op happens to be identical to an existing op, we could inadvertently miss out on CSE optimization opportunities”

Perhaps I wasn't clear earlier: Unlike C/C++, variables here do not undergo multiple assignments. Once an op is inserted into the ScopedHashTable, its value remains constant. Therefore, you don't need to worry about other operations being deleted—multiple erasure attempts are perfectly safe and harmless.

My English might not be as good as you think, so if anything is unclear, please let me know.

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