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

Added LOC info to TTRT Perf output #1401

Merged
merged 7 commits into from
Dec 2, 2024
Merged

Added LOC info to TTRT Perf output #1401

merged 7 commits into from
Dec 2, 2024

Conversation

vprajapati-tt
Copy link
Contributor

vprajapati-tt
vprajapati-tt commented Nov 25, 2024
View reviewed changes
include/ttmlir/Target/Utils/MLIRToFlatbuffer.h
Comment on lines +481 to +486

mlir::OpPrintingFlags flags;
flags.enableDebugInfo(); // Enable the loc dumping
module->print(os, flags);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Printing the MLIR module with debug info into the flatbuffer. I'm not sure if this is the behavior we want, but it could be useful to show the locations that are being propagated in the results.

@vprajapati-tt vprajapati-tt mentioned this pull request Nov 25, 2024
Closed
@vprajapati-tt vprajapati-tt linked an issue Nov 25, 2024 that may be closed by this pull request
Add TTIR/TTNN/TTMetal op location names to perf csv output report #1160
Closed
@vprajapati-tt vprajapati-tt mentioned this pull request Nov 25, 2024
Merged
@vprajapati-tt
Copy link
Contributor Author

Not merging in due to some weird quirk where this PR causes process_ops to return a faulty ops_perf_results.csv file that is missing rows. CC: @tapspatel, @jnie-TT I think you mentioned a similar bug before

@vprajapati-tt
Copy link
Contributor Author

Not merging in due to some weird quirk where this PR causes process_ops to return a faulty ops_perf_results.csv file that is missing rows. CC: @tapspatel, @jnie-TT I think you mentioned a similar bug before

Fixed! Will merge in on green check.

@nsmithtt
Copy link
Contributor

nsmithtt commented Dec 2, 2024 •
edited
Loading

FYI @ctodTT, I think this loc_info plumbing through flatbuffer that Vraj added is exactly what we want to use for golden IDs

@vprajapati-tt vprajapati-tt merged commit 139c7cc into main Dec 2, 2024
20 checks passed
@vprajapati-tt vprajapati-tt deleted the vprajapati/issue-1347 branch December 2, 2024 18:04
@tapspatel
Copy link
Contributor

thanks @vprajapati-tt ! This looks great. One comment inlined

tapspatel
tapspatel reviewed Dec 4, 2024
View reviewed changes
runtime/tools/python/ttrt/common/perf.py

from ttrt.common.util import *
from ttrt.common.query import Query


def get_loc_data_hook(binary, programContext, opContext):
op_debug_str = ttrt.runtime.get_op_debug_str(opContext)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming we aren't using the hook to get the loc info, but just querying the Tracy message itself? Since this function is not called anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This keeps getting rebased in oh my god that's super annoying. I'll add a branch to drop it.

vprajapati-tt added a commit that referenced this pull request Dec 4, 2024
@vprajapati-tt
Added fix for redundant constant nodes + small carry over from #1401
0502553
@vprajapati-tt vprajapati-tt mentioned this pull request Dec 4, 2024
Merged
vprajapati-tt added a commit that referenced this pull request Dec 6, 2024
@vprajapati-tt
Added fix for redundant constant nodes + small carry over from #1401 (#…
04d4d51 
…1509)

* Added fix for redundant constant nodes + small carry over from #1401

* Switched from checking block to check against op
azecevicTT pushed a commit that referenced this pull request Dec 17, 2024
@vprajapati-tt @azecevicTT
Added fix for redundant constant nodes + small carry over from #1401 (#…
780087f 
…1509)

* Added fix for redundant constant nodes + small carry over from #1401

* Switched from checking block to check against op
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tapspatel tapspatel tapspatel left review comments

@jnie-TT jnie-TT jnie-TT approved these changes

@nsmithtt nsmithtt nsmithtt approved these changes

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

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

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

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@vprajapati-tt @nsmithtt @tapspatel @jnie-TT