[mlir][SliceAnalysis] Add an options object to forward and backward slice.

[mravishankar]

Summary

Add an options object to allow control of the slice computation (for
both forward and backward slice). This makes the ABI stable, and also
allows avoiding an assert that makes the slice analysis unusable for
operations with multiple blocks.

[hanchung]

mlir/lib/Analysis/SliceAnalysis.cpp

104–109

Let's merge two if-statement into one if-statement.

[mravishankar]

Rebase

[nicolasvasilache]

It seems quite dangerous to just drop everything on the floor when we have != 1 regions / blocks.

Additionally, there are no tests in this PR.

Not sure what you are trying to achieve here but you may be in a "this needs a better algorithm" territory.

[mravishankar]

In D151520#4383339, @nicolasvasilache wrote:

It seems quite dangerous to just drop everything on the floor when we have != 1 regions / blocks.

I am not changing the behavior. I was getting an assert if the region had more than 1 block. Makes it impossible to use this with a func.func op with control flow.

Additionally, there are no tests in this PR.

Not sure what you are trying to achieve here but you may be in a "this needs a better algorithm" territory.

Better algorithm is definitely needed. But I am not sure what that looks like. I am just trying to use the existing slice function for a very trivial use case and being foiled by an assert. So, if having the assert is important I can maybe adapt the existing setup to add a SliceOptions object. It can assert by default, but opt-in to avoid the assert for people to do at their risk (I can add test. Didnt add cause I expected questions like above to be asked, so was waiting for more input to see where we go).

[mravishankar]

Add an options object to allow side stepping the assert.

[mravishankar]

Rebase

[mravishankar]

Add some comments.

[nicolasvasilache]

mlir/include/mlir/Analysis/SliceAnalysis.h

42 ↗

(On Diff #528712)

Given the behavior change I am seeing here, I would rename as omitBlockArguments and rephrase as:

When omitBlockArguments is true, the backward slice computation omits traversing any block arguments.
When omitBlockArguments is false, the backward slice computation traverses block arguments and asserts that the parent op has a single region with a single block.

[mravishankar]

Address comments.

[mravishankar]

Rebase

mlir/lib/Analysis/SliceAnalysis.cpp

if (auto *definingOp = operand.getDefiningOp()) {
       if (backwardSlice->count(definingOp) == 0)
         getBackwardSliceImpl(definingOp, backwardSlice, filter);
     } else if (auto blockArg = dyn_cast<BlockArgument>(operand)) {
       Block *block = blockArg.getOwner();
       Operation *parentOp = block->getParentOp();
       // 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 &&
-               parentOp->getRegion(0).getBlocks().size() == 1);
+        if (parentOp->getNumRegions() == 1 &&
+            parentOp->getRegion(0).getBlocks().size() == 1) {
           getBackwardSliceImpl(parentOp, backwardSlice, filter);
         }
+      }
     } else {
       llvm_unreachable("No definingOp and not a block argument.");
     }
   }
 
   backwardSlice->insert(op);
 }