[Mlir-commits] [mlir] [mlir][Analysis] Remove return value from `getBackwardSlice` (PR #163749)

[Matthias Springer]

matthias-springer wrote:

I think the next step is to figure out what to do with this TODO:
```c++
      // TODO: determine whether we want to recurse backward into the other
      // blocks of parentOp, which are not technically backward unless they flow
      // into us. For now, just bail.
      if (parentOp && !backwardSlice->contains(parentOp)) {
        if (!parentOp->hasTrait<OpTrait::IsIsolatedFromAbove>() &&
            parentOp->getNumRegions() == 1 &&
            parentOp->getRegion(0).hasOneBlock()) {
          return getBackwardSliceImpl(parentOp, visited, backwardSlice,
                                      options);
        }
      }
```

Would this indicate a potential failure condition?

I'm having trouble understanding why the code is written like that today.
- Why check for `parentOp->getRegion(0).hasOneBlock()`? Was the intention to check whether the block is the entry block? (Also, note that the implementation does not support unstructured control flow.)
- Why check for `parentOp->getNumRegions() == 1`? This seems arbitrary to me. What makes ops with a single region special? (Also, note that the implementation does not support region control flow.)
- Why check for `!parentOp->hasTrait<OpTrait::IsIsolatedFromAbove>()`? I read through #158135, but it still does not make sense to me. Why does it matter whether the operation is isolated from above? We're jumping from a region to its parent op in this traversal. I'm not sure what IsolatedFromAbove has to do with that? Maybe @IanWood1 can shed some light.

It feels like the handling of block argument was designed with implicit assumptions from downstream projects. Maybe the entire block argument handling should be delegated to a lambda in `BackwardSliceOptions`, so that downstream projects have better control over the traversal rules.


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