[Mlir-commits] [mlir] [mlir][CSE] Pre-process trivially dead ops (NFC) (PR #191135)

[lonely eagle]

================
@@ -308,7 +301,16 @@ LogicalResult CSEDriver::simplifyOperation(ScopedMapTy &knownValues,
 
 void CSEDriver::simplifyBlock(ScopedMapTy &knownValues, Block *bb,
                               bool hasSSADominance) {
-  for (auto &op : *bb) {
+  for (auto &op : llvm::make_early_inc_range(*bb)) {
+    // If the operation is already trivially dead just add it to the erase list.
+    // This also avoids calling `simplifyRegion` on dead region ops
+    // unnecessarily.
+    if (isOpTriviallyDead(&op)) {
----------------
linuxlonelyeagle wrote:

In the first case, if you run the canonicalization pass, you definitely won't have any dead code. CSE work success.

In the second case, if you don't run the canonicalization pass, there might be dead code. If its parent op (a region op) is essentially the same as another region op, CSE will fail. In other cases, it should succeed.

Why didn't I delete the dead code directly here?
Because we could run the canonicalization pass in advance, which would definitely make the CSE succeed.
Deleting the dead op allows the case below to run successfully.
```
module {
  func.func @cse_multiple_regions_with_dead_op(%arg0: i1, %arg1: tensor<5xf32>) -> (tensor<5xf32>, tensor<5xf32>) {
    %0 = scf.if %arg0 -> (tensor<5xf32>) {
      %2 = tensor.empty() : tensor<5xf32>
      scf.yield %2 : tensor<5xf32>
    } else {
      scf.yield %arg1 : tensor<5xf32>
    }
    %1 = scf.if %arg0 -> (tensor<5xf32>) {
      %2 = tensor.empty() : tensor<5xf32>
      scf.yield %2 : tensor<5xf32>
    } else {
      scf.yield %arg1 : tensor<5xf32>
    }
    return %0, %1 : tensor<5xf32>, tensor<5xf32>
  }
}
```
But in the case, `arith.add` was added here, so we can delete it. However, arith.constant then becomes a dead op as well, and we would need to continue deleting it for CSE to succeed. But since `arith.constant` is already in the ScopedMap, we cannot delete it; otherwise, the ScopedMap will crash because `arith.constant` has already been inserted into it. To fix the issue, I introduce this, https://github.com/llvm/llvm-project/pull/191394.But I also think this idea is not clever enough. I really want to do this right, so let’s leave this issue for now to be conservative. I will look into it later when I have the time.
```
func.func @cse_multiple_regions_with_dead_op(%c: i1, %t: tensor<5xf32>) -> (tensor<5xf32>, tensor<5xf32>) {
  %r1 = scf.if %c -> (tensor<5xf32>) {
    %0 = tensor.empty() : tensor<5xf32>
    %1 = arith.constant 1: index
    // we add the `arith.add`
    %2 = arith.addi %1, %1 : index
    scf.yield %0 : tensor<5xf32>
  } else {
    scf.yield %t : tensor<5xf32>
  }
  %r2 = scf.if %c -> (tensor<5xf32>) {
    %0 = tensor.empty() : tensor<5xf32>
    scf.yield %0 : tensor<5xf32>
  } else {
    scf.yield %t : tensor<5xf32>
  }
  return %r1, %r2 : tensor<5xf32>, tensor<5xf32>
}
```

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