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

[William Moses]

wsmoses wrote:

Paging all this back in, but trying to put together what's happening:

so the current state of the code here will not result in the crash as it has been changed since after my original PR, it seems (though in a way that perhaps changes behavior?).

Specifically the prior code was something like:

```
if (auto *definingOp = value.getDefiningOp()) {
...
} else if (auto blockArg = dyn_cast<BlockArgument>(value)) {
    ...
  // 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->count(parentOp) == 0) {
    assert(parentOp->getNumRegions() == 1 &&
           llvm::hasSingleElement(parentOp->getRegion(0).getBlocks()));
    getBackwardSliceImpl(parentOp, backwardSlice, options);
  }
} else {
  llvm_unreachable("No definingOp and not a block argument.");
}
```

the cause of the failure was the assertion above, where the blockargument's parent had more than one region.

my PR changed this to instead return failure in the case that the assertion would have failed, instead looking like:

```
if (auto *definingOp = value.getDefiningOp()) {
...
  return success();
} else if (auto blockArg = dyn_cast<BlockArgument>(value)) {
      if (parentOp && backwardSlice->count(parentOp) == 0) {
        if (parentOp->getNumRegions() == 1 &&
            llvm::hasSingleElement(parentOp->getRegion(0).getBlocks())) {
          return getBackwardSliceImpl(parentOp, backwardSlice, options);
        }
      }
    }
}

// This failure is hit in the case that it wasn't a supported blockargument, in that case
return failure();
```

The current code (prior to this PR, presumably changed between my PR and now), however, will never hit the failure, instead silently not adding anything if that assertion-case isn't hit during the blockargument, looking like as follows.

```

if (auto *definingOp = value.getDefiningOp()) {
...
} else if (auto blockArg = dyn_cast<BlockArgument>(value)) {
      if (parentOp && backwardSlice->count(parentOp) == 0) {
        if (parentOp->getNumRegions() == 1 &&
            llvm::hasSingleElement(parentOp->getRegion(0).getBlocks())) {
         getBackwardSliceImpl(parentOp, backwardSlice, options);
        }
      }
    }
} else {
   return failure();
}

return success();
```

so indeed presently in the implementation this isn't ever expected to fail. 

I'm not sufficiently familiar presently with what the analysis is doing to assess whether the move of the assertion to the if statement without failure is legal (or if it is a correctness problem). However, considering the comment of:
```
      // 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.
```

remains, that's at least mildly concerning from a correctness standpoint.

It looks like the correctness condition was accidentally removed here (https://github.com/llvm/llvm-project/pull/142076) and there is an unmerged PR here, to restore the correctness condition from earlier (https://github.com/llvm/llvm-project/pull/142223)

If the correctness condition isn't needed and indeed there is no way this can fail, I'm fine removing the logicalresult. However, if it is as it appears that there are parts of backwards slice analysis marked as a TODO and unimplemented (resulting in a failure of the analysis), I would advocate for leaving the logicalresult.

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