[Mlir-commits] [mlir] [mlir][Analysis][NFC] Improve `RegionBranchOpInterface` API usage (PR #173983)

[Matthias Springer]

================
@@ -115,8 +69,11 @@ mlir::getControlFlowPredecessors(Value value) {
       return std::nullopt;
     // Add the control flow predecessor operands to the work list.
     RegionSuccessor region(regionOp, regionOp->getResults());
-    SmallVector<Value> predecessorOperands = getRegionPredecessorOperands(
-        regionOp, region, opResult.getResultNumber());
+    SmallVector<Value> predecessorOperands;
+    // TODO: This assumes that there are no non-forwarded op results in front
+    // of the forwarded op results.
----------------
matthias-springer wrote:

Sorry, the right term is "successor input". I updated the comment.

The second parameter of `getPredecessorValue` indexes into `getEntrySuccessorOperands` /`getSuccessorOperands`. The indexing starts with `0`. The problem here is that we are simply taking the op result number.

For example, let's assume that `scf.for` returns an additional value that is not related to an init_arg. Let's say it returns the number of iterations. (It does not really make sense to design the op like that, this is just for illustration purposes.)

```
%num_iter, %loop_carried = scf.for ... iter_args(%arg0 = %0) { ... }
//         ^^^^^^^^^^^^^
//          op result 1
```

Now we are calling `getControlFlowPredecessors(%loop_carried)` here. This will call `regionBranchOp.getPredecessorValues(..., 1, ...)`, which cause an out-of-bounds array access inside of `getPredecessorValues`. (The same thing happens with the original implementation before this PR.)

While we may not have such ops (at least in upstream MLIR) at the moment, the `RegionBranchOpInterface` allows them.



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