[Mlir-commits] [mlir] Allow SymbolUserOpInterface operators to be used in RemoveDeadValues Pass (PR #117405)

[M. Zeeshan Siddiqui]

================
@@ -577,10 +577,8 @@ void RemoveDeadValues::runOnOperation() {
   WalkResult acceptableIR = module->walk([&](Operation *op) {
     if (op == module)
       return WalkResult::advance();
-    if (isa<BranchOpInterface>(op) ||
-        (isa<SymbolUserOpInterface>(op) && !isa<CallOpInterface>(op))) {
-      op->emitError() << "cannot optimize an IR with "
-                         "non-call symbol user ops or branch ops\n";
+    if (isa<BranchOpInterface>(op)) {
----------------
codemzs wrote:

In the `RemoveDeadValues` pass, there's an issue when a value is used **only** as a block argument in branch operations (e.g., `cf.cond_br`) but has **no uses** within the successor blocks. The pass incorrectly (?) treats such values as dead and removes their definitions. However, the branch operation still references these values as block arguments, leading to **null** operands and compiler errors like:

Example IR:

```
func.func @dont_touch_unacceptable_ir_has_cleanable_simple_op_with_branch_op(%arg0: i1) {
  %non_live = arith.constant 0 : i32
  cf.cond_br %arg0, ^bb1(%non_live : i32), ^bb2(%non_live : i32) {tag = "br"}
^bb1(%non_live_0 : i32):
  cf.br ^bb3
^bb2(%non_live_1 : i32):
  cf.br ^bb3
^bb3:
  return
}
```

Compiler Error:
```
error: unexpected error: null operand found
cf.cond_br %arg0, ^bb1(%non_live : i32), ^bb2(%non_live : i32)
```

Detailed Explanation:

- What Happens:

  - The value `%non_live` is defined but only used as a block argument in the branch operation `cf.cond_br`.

  - It is not used inside the successor blocks `^bb1` and `^bb2`.
  - The `RemoveDeadValues` pass incorrectly considers `%non_live` as dead and removes its definition.
  - The `cf.cond_br` operation still references `%non_live`, resulting in a null operand error.
 - Why It Doesn't Occur When Value Is Used Inside Blocks:
   - If `%non_live` is used within the successor blocks (e.g., in a `memref.store `operation with memory effects), the pass correctly identifies it as live.
   - Its definition is preserved, and no error occurs.
 - Cause of the Problem:
   - The liveness analysis in the `RemoveDeadValues` pass does not consider values used only as block arguments in branch operations to be live if they have no uses within the successor blocks.
   - This leads to the removal of their definitions while they are still referenced by the branch operations.

- Proposed Solution:
  - Update the `RemoveDeadValues` pass to treat values used as block arguments in branch operations as live, even if they have no uses within the successor blocks **OR** remove the branches altogether assuming it has no uses.
  - This ensures that the definitions of such values are not removed, preventing null operand errors.
  
 The criteria for test PASS/FAIL is compiler generating the correct IR with no errors, example the below IR remains the same and compiler does not produce an error:

``` 
 func.func @test_2_RegionBranchOpInterface_type_1.b(%arg0: memref<i32>, %arg1: memref<i32>, %arg2: i1) {
  %c0_i32 = arith.constant 0 : i32
  cf.cond_br %arg2, ^bb1(%c0_i32 : i32), ^bb2(%c0_i32 : i32) {tag = "br"}
^bb1(%0 : i32):
  memref.store %0, %arg0[] : memref<i32>
  cf.br ^bb3
^bb2(%1 : i32):
  memref.store %1, %arg1[] : memref<i32>
  cf.br ^bb3
^bb3:
  return
}
```


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