Skip to content
New issue

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

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

Already on GitHub? # 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.

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

Successfully merging this pull request may close these issues.

2 participants