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.

Sign up for GitHub

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

Jump to bottom

Slice improvements #17615

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

Slice improvements #17615

wants to merge 10 commits into from
+479 −340

Conversation

amorrisonTT
Copy link
Contributor

@amorrisonTT amorrisonTT commented Feb 5, 2025 •
edited
Loading

Ticket

#17351
#16966

Problem description

A few bugs/missing features/bad practices in slice:

  • Python style list splicing slices do not work when we want to slice off a single dimension eg) tensor[0, :::]
  • When begins is unaligned for row major slice, we get PCC errors
  • 1D row major strided slice gets a hang
  • Host side fall back to unpad when the tensor is on host
  • No support for non-tile aligned begins/ends for tiled slice

What's changed

  • Add Python style list splicing for single dimensions
  • Fix row major slice on unaligned begins
  • Add support for 1D strided RM slice
  • Remove host side code
  • Fallback to RM slice when tiled slice has unaligned begins/ends

Sweep tests at 100% pass. Results:
slice_forge_sweeps.csv
slice_rm_pytorch_sweep.csv
slice_tiled_pytorch2_sweep.csv

Checklist

@amorrisonTT amorrisonTT marked this pull request as ready for review February 5, 2025 21:32
@amorrisonTT amorrisonTT changed the title DRAFT: Slice improvements Slice improvements Feb 5, 2025
ntarafdar
ntarafdar reviewed Feb 5, 2025
View reviewed changes
...ment/slice/device/kernels/dataflow/slice_reader_unary_unpad_dims_rm_interleaved_start_id.cpp
@@ -35,7 +38,12 @@ void kernel_main() {
for (uint32_t i = 0; i < num_read_per_barrier and sticks_read < num_sticks_per_core; ++i) {
sticks_read++;
uint64_t src_noc_addr = get_noc_addr(src_stick_id, s0);
noc_async_read(src_noc_addr, src_buffer_l1_addr, unpadded_stick_size);
noc_async_read(src_noc_addr, src_buffer_l1_addr, read_size);
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that we are using this common memmove

Copy link
Contributor

Choose a reason for hiding this comment

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

I do as well

@ntarafdar
Copy link
Contributor

LGTM , I'll let @sjameelTT sign off on this from TMs

jvegaTT
jvegaTT approved these changes Feb 5, 2025
View reviewed changes
Copy link
Contributor

@jvegaTT jvegaTT left a comment

Choose a reason for hiding this comment

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

This is amazing, great work!

@amorrisonTT amorrisonTT mentioned this pull request Feb 5, 2025
Open
6 tasks
sjameelTT
sjameelTT approved these changes Feb 5, 2025
View reviewed changes
Copy link
Contributor

@sjameelTT sjameelTT left a comment

Choose a reason for hiding this comment

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

Thanks for (very quickly) fixing up all the remaining issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ntarafdar ntarafdar ntarafdar left review comments

@nardoTT nardoTT nardoTT approved these changes

@jvegaTT jvegaTT jvegaTT approved these changes

@sjameelTT sjameelTT sjameelTT approved these changes

@ayerofieiev-tt ayerofieiev-tt Awaiting requested review from ayerofieiev-tt ayerofieiev-tt is a code owner

@dmakoviichuk-tt dmakoviichuk-tt Awaiting requested review from dmakoviichuk-tt dmakoviichuk-tt is a code owner

@rfurko-tt rfurko-tt Awaiting requested review from rfurko-tt rfurko-tt is a code owner

@cfjchu cfjchu Awaiting requested review from cfjchu cfjchu is a code owner

@TT-BrianLiu TT-BrianLiu Awaiting requested review from TT-BrianLiu TT-BrianLiu is a code owner

@razorback3 razorback3 Awaiting requested review from razorback3 razorback3 is a code owner

@dongjin-na dongjin-na Awaiting requested review from dongjin-na dongjin-na is a code owner

@bbradelTT bbradelTT Awaiting requested review from bbradelTT bbradelTT is a code owner

@jaykru-tt jaykru-tt Awaiting requested review from jaykru-tt jaykru-tt is a code owner

@yugi957 yugi957 Awaiting requested review from yugi957 yugi957 is a code owner

@llongTT llongTT Awaiting requested review from llongTT llongTT is a code owner

At least 0 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@amorrisonTT @ntarafdar @sjameelTT @jvegaTT @nardoTT