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

[LinalgExt] Update scatter to allow dropping unit dims #19704

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

IanWood1
Copy link
Contributor

@IanWood1 IanWood1 commented Jan 15, 2025

Changes scatter to enforce that updates is only batch dims + contiguous slice. This allows us to drop fold the unit dimension from indices when index_depth is 1.

@IanWood1 IanWood1 force-pushed the remove_scatter_unit_index_depth branch from 92776de to dc05f7a Compare January 15, 2025 18:15
@IanWood1 IanWood1 requested a review from qedawkins January 15, 2025 18:40
Copy link
Contributor

@qedawkins qedawkins left a comment

Choose a reason for hiding this comment

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

No obvious issues with the verifier changes to me. Haven't looked at the rest of the changes but can take a look once ready for a full review.

const auto originalSliceRank = originalType.getRank() - indexDepth;
if (originalSliceRank < 0) {
return op->emitOpError(
"expected original rank to be greater or equal to index depth");
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this error should be more explicit in terms of slize rank being >= 0.

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.

Looks ok overall. Is there a sequencing thing that you dropped the implementation of the DropScatter*Unit* ? Your message says so above, so assuming its just a sequencing thing. If you do want to drop it, just drop it from the pattern list completely and add it back later instead of making the implementation dummy.

also change expected result for e2e correctness (maybe delete?)

Signed-off-by: Ian Wood <[email protected]>
@IanWood1
Copy link
Contributor Author

Looks ok overall. Is there a sequencing thing that you dropped the implementation of the DropScatter*Unit* ? Your message says so above, so assuming its just a sequencing thing. If you do want to drop it, just drop it from the pattern list completely and add it back later instead of making the implementation dummy.

Yeah sorry that was confusing. The pattern I added and the one I deleted are 2 separate ones (the pre-existing one is a noop because there can no longer be unit dims between updates batch rank and slice)

Signed-off-by: Ian Wood <[email protected]>
@IanWood1 IanWood1 marked this pull request as ready for review January 16, 2025 19:57
@IanWood1 IanWood1 requested a review from hanhanW as a code owner January 16, 2025 19:57
@IanWood1
Copy link
Contributor Author

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.

3 participants