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

#17972 and #17975 Fixing PCC and Program Cache issues in Repeat and Expand #18002

Merged

jvegaTT merged 17 commits into main from jvega/fixing_repeat_regression

Feb 20, 2025

Contributor

jvegaTT commented Feb 19, 2025 •

edited

Loading

Ticket

Problem description

This PR closes two P0 errors by applying bug fixes to the repeat program factory and giving repeat program cache support.

What's changed

Program factory changes to repeat
Adding Program Cache testing to the CI pipelines for repeat
Removed redundant CI tests in Repeat to help improve CI pipeline times

Checklist

All post commit CI passes. Submitted rerun after PR changes
Blackhole Post commit CI passes (if applicable)
Model regression CI passes (if applicable). Submitted
Device performance regression CI passes (if applicable)
(For models and ops writers) Full new models tests CI passes (if applicable)
New/Existing tests provide coverage for changes
T3K Demo. Submitted

jvegaTT added 5 commits

February 18, 2025 21:55


          #0: add minimally failing test

a7e9454


          #17975: Program Cache fix for expand and repeat

4e05c7a


          #17975: Adding the program cache tests for repeat

14b590b


          #0: removing a now incorrect comment

104adad


          #0:Removing the minimally failing test

74f4c29

jvegaTT requested review from ayerofieiev-tt, dmakoviichuk-tt, rfurko-tt, cfjchu, TT-BrianLiu, razorback3, dongjin-na, bbradelTT, ntarafdar, sjameelTT, jaykru-tt, yugi957, llongTT and nardoTT as code owners

February 19, 2025 15:51

ntarafdar approved these changes

View reviewed changes

ayerofieiev-tt reviewed

View reviewed changes

ttnn/cpp/ttnn/operations/data_movement/repeat/device/host/repeat_program_factory.cpp Outdated Show resolved Hide resolved

TT-BrianLiu requested changes

View reviewed changes

Contributor

TT-BrianLiu left a comment

I don't think your implementation of program callback is correct. You only need to update the args that actually change, but it seems like you copied most of the program setup itself, which is really expensive.

Another thing is, you can capture a lot more args (eg. cores) from the program setup instead of recalculating everything (unless they are expected to change).

tests/ttnn/unit_tests/operations/test_repeat.py Outdated Show resolved Hide resolved

tests/ttnn/unit_tests/operations/test_repeat.py Outdated Show resolved Hide resolved

tests/ttnn/unit_tests/operations/test_repeat.py Outdated Show resolved Hide resolved

ttnn/cpp/ttnn/operations/data_movement/repeat/device/host/repeat_program_factory.cpp Outdated Show resolved Hide resolved

ttnn/cpp/ttnn/operations/data_movement/repeat/device/host/repeat_program_factory.cpp Outdated Show resolved Hide resolved


          #0: Applying PR review comments regarding the test_repeat

93b87b3

dmakoviichuk-tt reviewed

View reviewed changes

tests/ttnn/unit_tests/operations/test_repeat.py Outdated Show resolved Hide resolved

dmakoviichuk-tt reviewed

View reviewed changes

Contributor

dmakoviichuk-tt left a comment

Please add explanation on how does it break repeat calls with the same parameters in a cycle. I currently don't' understand why hash is broken in this case.

TT-BrianLiu reviewed

View reviewed changes

tests/ttnn/unit_tests/operations/test_repeat.py Outdated Show resolved Hide resolved

aliuTT reviewed

View reviewed changes

ttnn/cpp/ttnn/operations/data_movement/repeat/device/host/repeat_program_factory.cpp Outdated Show resolved Hide resolved

ayerofieiev-tt mentioned this pull request

Improve usability of Program Cache handling in Device Operations #18015

Open

jvegaTT added 2 commits

February 19, 2025 18:45


          #0: Adding more tests and reducing override complexity

ac54306


          #0: Further simplifying runtime args callback

919334e


          #0: re-adding bfloat8 tests

3cc221d

ayerofieiev-tt reviewed

View reviewed changes

tests/ttnn/unit_tests/operations/test_repeat.py Outdated Show resolved Hide resolved

ayerofieiev-tt approved these changes

View reviewed changes


          #0: Applying further program cache simplification

71d458f

TT-BrianLiu reviewed

View reviewed changes

tests/ttnn/unit_tests/operations/test_repeat.py Outdated Show resolved Hide resolved

tests/ttnn/unit_tests/operations/test_repeat.py Outdated Show resolved Hide resolved

tests/ttnn/unit_tests/operations/test_repeat.py Show resolved Hide resolved

tests/ttnn/unit_tests/operations/test_repeat.py Outdated Show resolved Hide resolved

tests/ttnn/unit_tests/operations/test_repeat.py Outdated Show resolved Hide resolved

jvegaTT added 5 commits

February 19, 2025 20:51


          #0: Cleaning up testing

6a1c3ec


          #0: Cleaning up the tests a bit more

9e0b091


          Merge branch 'main' into jvega/fixing_repeat_regression

6f35ab1


          #0: Added num program cache entries assert to test

8b7e3f6


          Merge branch 'jvega/fixing_repeat_regression' of https://github.com/t…

…enstorrent/tt-metal into jvega/fixing_repeat_regression

TT-BrianLiu approved these changes

View reviewed changes

bbradelTT approved these changes

View reviewed changes

tests/ttnn/unit_tests/operations/test_repeat.py Outdated Show resolved Hide resolved


          #0: changing the test name

7dbaee5

dmakoviichuk-tt approved these changes

View reviewed changes

rfurko-tt reviewed

View reviewed changes

Contributor

rfurko-tt left a comment

LGTM, thanks!

rfurko-tt approved these changes

View reviewed changes


          Merge branch 'main' into jvega/fixing_repeat_regression

aff3db9

jvegaTT merged commit 705b94d into main

12 checks passed

jvegaTT deleted the jvega/fixing_repeat_regression branch

February 20, 2025 03:53

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

Reviewers

aliuTT aliuTT left review comments

TT-BrianLiu TT-BrianLiu approved these changes

ntarafdar ntarafdar approved these changes

bbradelTT bbradelTT approved these changes

ayerofieiev-tt ayerofieiev-tt approved these changes

dmakoviichuk-tt dmakoviichuk-tt approved these changes

rfurko-tt rfurko-tt approved these changes

cfjchu Awaiting requested review from cfjchu cfjchu is a code owner

razorback3 Awaiting requested review from razorback3 razorback3 is a code owner

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

sjameelTT Awaiting requested review from sjameelTT sjameelTT is a code owner

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

yugi957 Awaiting requested review from yugi957 yugi957 is a code owner

llongTT Awaiting requested review from llongTT llongTT is a code owner

nardoTT Awaiting requested review from nardoTT nardoTT is a code owner

Labels

None yet