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

#3764: Device side opID support #9830

Closed
wants to merge 2 commits into from
Closed

#3764: Device side opID support #9830

wants to merge 2 commits into from

Conversation

mo-tenstorrent
Copy link
Contributor

@mo-tenstorrent mo-tenstorrent commented Jun 28, 2024 •
edited
Loading

Ticket

3764

Problem description

Device profiler data did not know which op the running kernels belonged to.

What's changed

  • opID is now updated for cached and uncached FD enqueue programs and for SD LaunchProgram.
  • BRISC FW sets the ID for all riscs ops

With this:

  • Device side opID is verified against the expected ID extracted by host
  • Tracy device zones include opID
Screenshot 2024-06-28 at 3 11 12 PM

Checklist

  • Post commit CI passes
  • uBenchmark
  • Device Perf
  • T3K profiler

@mo-tenstorrent mo-tenstorrent mentioned this pull request Jul 4, 2024
Closed
aliuTT
aliuTT reviewed Jul 4, 2024
View reviewed changes
tt_metal/tools/profiler/kernel_profiler.hpp
for (int i = GUARANTEED_MARKER_1_H; i < CUSTOM_MARKERS; i ++)
{
//TODO(MO): Clean up magic numbers
//TODO(MO): Clean up magic numbers
Copy link
Contributor

Choose a reason for hiding this comment

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

where are these numbers getting used? Is there risk that device already holds these values on start, and FW jumps ahead of init?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am referring to the 0x80000000. They are pretty settled at this point. I will clean them up in another PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are in init, so we are flushing out anything that might be there from the previous run.

aliuTT
aliuTT reviewed Jul 4, 2024
View reviewed changes
tt_metal/hw/inc/dev_msgs.h
@@ -65,6 +65,8 @@ struct launch_msg_t { // must be cacheline aligned
volatile uint16_t watcher_kernel_ids[DISPATCH_CLASS_MAX_PROC];
volatile uint16_t ncrisc_kernel_size16; // size in 16 byte units

volatile uint16_t host_assigned_op_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

@pgkeller was changing launch_msg_t in his PR here: https://github.com/tenstorrent/tt-metal/pull/9781/files#diff-d606266ae23fd80f4fa4f80ea161f8b17ff937bbf94cce50cfd731755d0a491b

He suspects that moving run field around is causing some issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm interesting, I did not see any hangs on CI with the addition of the two bytes. I will coordinate before merging this in.

aliuTT
aliuTT reviewed Jul 4, 2024
View reviewed changes
tt_metal/impl/program/program.hpp
@@ -77,11 +77,13 @@ class Program {
Program(Program &&other) = default;
Program& operator=(Program &&other) = default;

void set_global_id(uint64_t id);
Copy link
Contributor

@aliuTT aliuTT Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is this only called by ttnn and tteager? So other programs (i.e. generated from our backend) don't have a global id?

Copy link
Contributor Author

@mo-tenstorrent mo-tenstorrent Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Right, this is for use by higher levels. Making direct tt_metal calls can ignore this and op id field will show up as zero ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i.e. this is for assigning unique ids for op_report

Copy link
Contributor

Choose a reason for hiding this comment

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

Where is it initialized to 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good find, I thought I added it. Will add to initializer list for program

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@mo-tenstorrent mo-tenstorrent force-pushed the mo/3764_device_share_opID branch 2 times, most recently from 2583943 to e642070 Compare July 11, 2024 16:06
@pgkeller
#10107: Fix hangs w/ launch_msg size >32bytes
83d596e 
Change default MMIO TLB ordering from posted to strict (will have a negative
perf impact)
Split launch message into kernel_config and go
Write these separately from host w/ an sfence between
ubcheema
ubcheema reviewed Jul 11, 2024
View reviewed changes
tt_metal/hw/inc/dev_msgs.h
@@ -91,7 +93,17 @@ struct launch_msg_t { // must be cacheline aligned
volatile uint8_t dispatch_core_x;
volatile uint8_t dispatch_core_y;
volatile uint8_t exit_erisc_kernel;
volatile uint8_t run; // must be in last cacheline of this msg
volatile uint8_t pad1;
volatile uint16_t pad2;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this 2 byte padding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to be 32-bit aligned on this struct, cache aligned.

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

@aliuTT aliuTT aliuTT left review comments

@ubcheema ubcheema ubcheema left review comments

@eyonland eyonland Awaiting requested review from eyonland

@arakhmati arakhmati Awaiting requested review from arakhmati

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

@xanderchin xanderchin Awaiting requested review from xanderchin

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

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

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

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

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

@TT-billteng TT-billteng Awaiting requested review from TT-billteng

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
@mo-tenstorrent @ubcheema @aliuTT @pgkeller @tt-aho