[Mlir-commits] [mlir] [mlir][scf] Fix WhileMoveIfDown with duplicated scf.condition operands (PR #219458)

[Alessandro Potenza]

https://github.com/alepot55 commented:

Built this on `11e915f2b75d` and ran your test case through `mlir-opt -canonicalize`. The miscompile is there and the patch fixes it.

Before, the after-region body is:

```mlir
%1 = "test.get_some_value1"() : () -> i32
"test.use0"(%1) : (i32) -> ()
"test.use1"(%arg0) : (i32) -> ()
```

After:

```mlir
%1 = "test.get_some_value1"() : () -> i32
"test.use0"(%1) : (i32) -> ()
"test.use1"(%1) : (i32) -> ()
```

`%arg0` in the first one is the else value forwarded through `scf.condition`, and the after region only runs when the condition is true, so the second forwarded position was reading the wrong branch. `mlir/test/Dialect/SCF` is green with the patch (45 passed, 1 expectedly failed).

Two things on the shape of the fix.

**The `collapsedIfResults` set looks unnecessary.** After the first `replaceAllUsesWith(ifResult, elseValue)` that result has no uses left, so a second call for the same index is a no-op. I dropped the set and reduced the third loop to

```cpp
for (auto [idx, ifOpIdx] : conditionToIfResult)
  rewriter.replaceAllUsesWith(ifOp->getResults()[ifOpIdx],
                              ifOp.elseYield()->getOperand(ifOpIdx));
```

and `mlir/test/Dialect/SCF` is unchanged, 45 passed and 1 expectedly failed. One fewer piece of state and one fewer `DenseSet`, unless it is guarding something I have not thought of.

**The comment on that loop asserts something worth spelling out.** It says any remaining use of an `ifOp` result is on the false path. That is true, but only because of where the op is: `ifOp` is `conditionOp->getPrevNode()`, so no other operation in the before-region block can use its results, and results of a region cannot escape it. The `assert` above the loop is the same claim, so a reader who wonders whether the wholesale replacement can hit something outside the condition has to reconstruct the argument twice. One clause pointing at `getPrevNode()` would settle it.

No reviewers are requested on this. Going by the file, @matthias-springer and @joker-eph have by far the most commits in `SCF.cpp` over the past year.

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