This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mravishankar on May 25 2023, 4:50 PM.

Details

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.

Diff Detail

Event Timeline

mravishankar created this revision.May 25 2023, 4:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2023, 4:50 PM
mravishankar requested review of this revision.May 25 2023, 4:50 PM
hanchung accepted this revision.May 26 2023, 2:10 PM
hanchung added inline comments.
mlir/lib/Analysis/SliceAnalysis.cpp
107–111

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

This revision is now accepted and ready to land.May 26 2023, 2:10 PM
nicolasvasilache requested changes to this revision.May 30 2023, 11:34 PM

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.

This revision now requires changes to proceed.May 30 2023, 11:34 PM

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).

Add an options object to allow side stepping the assert.

mravishankar retitled this revision from [mlir][SliceAnalysis] Remove conservative assert in backward slice computation. to [mlir][SliceAnalysis] Add an options object to forward and backward slice..Jun 5 2023, 11:44 PM
mravishankar edited the summary of this revision. (Show Details)
mravishankar marked an inline comment as done.

Add some comments.

nicolasvasilache accepted this revision.Jun 7 2023, 3:10 AM
nicolasvasilache added inline comments.
mlir/include/mlir/Analysis/SliceAnalysis.h
42

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.
This revision is now accepted and ready to land.Jun 7 2023, 3:10 AM

Address comments.