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

feat: __getitem__ logic for MLIR backend #779

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

mtsokol
Copy link
Collaborator

@mtsokol mtsokol commented Sep 24, 2024

Hi @hameerabbasi,

This PR adds __getitem__ logic so that tensor[:, :, ...] can be run. The current version preserves rank (and format).

For now unfortunately it's blocked by https://discourse.llvm.org/t/illegal-operation-when-slicing-csr-csc-coo-tensor/81404 and I'm not sure if SparseTensor dialect fully supports slices.

An interesting case is for example tensor[:, :] which just returns tensor but our ownership mechanism sees it as MLIR allocated object, where in the meantime it's still SciPy/NumPy that was passed in. I think the mechanism requires a tweak where calling MLIR ops (reshape, slices, elemwise) should also tell if it's MLIR allocated (thus requires a free) or just a reference to what was passed (SciPy/NumPy managed arrays).

@mtsokol mtsokol self-assigned this Sep 24, 2024
@hameerabbasi hameerabbasi changed the title ENH: __getitem__ logic for MLIR backend feat: __getitem__ logic for MLIR backend Sep 24, 2024
@github-actions github-actions bot added the enhancement Indicates new feature requests label Sep 24, 2024
@hameerabbasi
Copy link
Collaborator

I wonder if adding ndindex as a dependency makes sense?

Copy link

codspeed-hq bot commented Sep 24, 2024

CodSpeed Performance Report

Merging #779 will degrade performances by 54.63%

Comparing getitem-func (fbd4586) with main (db60537)

Summary

❌ 2 regressions
✅ 338 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main getitem-func Change
test_index_fancy[side=100-rank=1-format='coo'] 643.5 µs 1,418.3 µs -54.63%
test_index_slice[side=100-rank=2-format='gcxs'] 2.2 ms 2.7 ms -19.77%

Copy link
Collaborator

@hameerabbasi hameerabbasi left a comment

Choose a reason for hiding this comment

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

It seems the only test added here is skipped, due to a bug in LLVM. Let's keep this PR open until it adds new working functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants