Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Flow] Improve slice raising to handle dynamic and unit dims #14845

Closed
wants to merge 3 commits into from

Conversation

qedawkins
Copy link
Contributor

This enables the following raising cases:

  1. Dynamic dimensions able to be statically analyzed as matching the associated dimension in the output.
  2. Unit dimensions indexed by 0.

Dynamic dimensions are compared using the ValueBounds interface, however this currently has no pattern for analyzing tensor.expand/collapse shape ops, making this pattern interact poorly with FoldUnitExtentDims.

Also cleans up some of the test cases for slice raising.

Copy link
Contributor

@MaheshRavishankar MaheshRavishankar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to take a look a bit more here.

continue;
}
// Assume that the constant access is a rank reducing access.
// Constant accesses can either be rank reducing or an access into a unit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually limiting the cases covered? The constant access does not have to be for dimensions that are unit dim. You could have a constant access where the source of the unit dim could be anything. Then the constant (or any ssa value for that matter that is not related the Linalg op loops become the offset, and the size is 1.

I'd like to take a bit of a further look into this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment here is a bit misleading. It does have to either be a unit dim or rank reducing on the output though due to the constraints of a linalg op. If not one of those then it is a broadcast and thus can't be raised, but I think that should be captured properly. That said, I might not have thought through it properly and will come back to it later. I'm also starting to think that this intermediate linalg approach is nice for the cases that work, but there are certain classes of slices (exactly when we have an arbitrary computation of an index/size for slicing) that warrant a direct lowering. I'd like to see real world use cases for those before adding raisings for them though.

I'll add a test case for broadcast to make sure it isn't erroneously raised though.

This enables the following raising cases:
  1. Dynamic dimensions able to be statically analyzed as matching the
     associated dimension in the output.
  2. Unit dimensions indexed by 0.

Dynamic dimensions are compared using the ValueBounds interface, however
this currently has no pattern for analyzing tensor.expand/collapse shape
ops, making this pattern interact poorly with FoldUnitExtentDims.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants