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.

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

Better kernel debug support #691

Merged
merged 1 commit into from
Sep 16, 2024
Merged

Better kernel debug support #691

merged 1 commit into from
Sep 16, 2024

Conversation

nsmithtt
Copy link
Contributor

@nsmithtt nsmithtt commented Sep 14, 2024

  • Introduce debug singleton object used to hold debug environment information during runtime. This could be a good place to store new kinds of debug flags in the future for runtime.
  • Wire it up through ttrt.
  • loadKernelsFromDisk added to denote reloading of generated kernels from /tmp, from previous run, instead of loading them from the flatbuffer.
  • Name the kernels with program and location info from the MLIR graph. Now names look like:
                      /tmp/ttmlir_multiply_%5_tensix.cpp
                                  ^        ^  ^
                       Func name -/        |  |
                                           |  |
                         Result Value Loc -/  |
                                              |
         Thread type/info [noc, eth, tensix] -/
  • Usage with ttrt:
# Run ttrt per usual
ttrt run simple_eltwise.ttm

# List kernels
ls /tmp/ttmlir_*

# Edit kernel
vim /tmp/ttmlir_multiply_%5_tensix.cpp

# Reload kernels from disk
ttrt run --load-kernels-from-disk simple_eltwise.ttm

@nsmithtt nsmithtt force-pushed the nsmith/ttrt-kernel-debug branch 2 times, most recently from 38339d3 to f9a1a2c Compare September 14, 2024 19:23
- Introduce debug singleton object used to hold debug environment
  information during runtime.  This could be a good place to store new
  kinds of debug flags in the future for runtime.
- Wire it up through ttrt.
- loadKernelsFromDisk added to denote reloading of generated kernels
  from /tmp, from previous run, instead of loading them from the
  flatbuffer.
- Name the kernels with program and location info from the MLIR graph.
  Now names look like /tmp/ttmlir_multiply_%5_tensix.cpp
                                  ^        ^  ^
                       Func name -/        |  |
                                           |  |
                         Result Value Loc -/  |
                                              |
         Thread type/info [noc, eth, tensix] -/
@nsmithtt nsmithtt force-pushed the nsmith/ttrt-kernel-debug branch from f9a1a2c to 9e6f47c Compare September 15, 2024 03:30
Copy link
Contributor

@kmabeeTT kmabeeTT left a comment

Choose a reason for hiding this comment

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

Looks awesome. Was just realizing today that program name wasn't visible to kernels so this is timely and handy.

Copy link
Contributor

@pjanevskiTT pjanevskiTT left a comment

Choose a reason for hiding this comment

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

Looks great!

@nsmithtt nsmithtt merged commit e891e98 into main Sep 16, 2024
11 checks passed
@nsmithtt nsmithtt deleted the nsmith/ttrt-kernel-debug branch September 16, 2024 15:26
uazizTT pushed a commit that referenced this pull request Sep 16, 2024
- Introduce debug singleton object used to hold debug environment
  information during runtime.  This could be a good place to store new
  kinds of debug flags in the future for runtime.
- Wire it up through ttrt.
- loadKernelsFromDisk added to denote reloading of generated kernels
  from /tmp, from previous run, instead of loading them from the
  flatbuffer.
- Name the kernels with program and location info from the MLIR graph.
  Now names look like /tmp/ttmlir_multiply_%5_tensix.cpp
                                  ^        ^  ^
                       Func name -/        |  |
                                           |  |
                         Result Value Loc -/  |
                                              |
         Thread type/info [noc, eth, tensix] -/
vprajapati-tt pushed a commit that referenced this pull request Sep 16, 2024
- Introduce debug singleton object used to hold debug environment
  information during runtime.  This could be a good place to store new
  kinds of debug flags in the future for runtime.
- Wire it up through ttrt.
- loadKernelsFromDisk added to denote reloading of generated kernels
  from /tmp, from previous run, instead of loading them from the
  flatbuffer.
- Name the kernels with program and location info from the MLIR graph.
  Now names look like /tmp/ttmlir_multiply_%5_tensix.cpp
                                  ^        ^  ^
                       Func name -/        |  |
                                           |  |
                         Result Value Loc -/  |
                                              |
         Thread type/info [noc, eth, tensix] -/
@rpavlovicTT
Copy link
Contributor

@nsmithtt I noticed we dump only 1 kernel per thread type, but we create data movement kernels for every core independently. And maybe we'll have split compute kernels per core groups too. Should we dump all of them?

@nsmithtt
Copy link
Contributor Author

@nsmithtt I noticed we dump only 1 kernel per thread type, but we create data movement kernels for every core independently. And maybe we'll have split compute kernels per core groups too. Should we dump all of them?

Good point, we should encode the core grid shape / offset in the string name, i.e. one kernel per entry in the core_range_set. If you have time to add this it'd be much appreciated!

@rpavlovicTT
Copy link
Contributor

@nsmithtt I noticed we dump only 1 kernel per thread type, but we create data movement kernels for every core independently. And maybe we'll have split compute kernels per core groups too. Should we dump all of them?

Good point, we should encode the core grid shape / offset in the string name, i.e. one kernel per entry in the core_range_set. If you have time to add this it'd be much appreciated!

Sure, #794 issue tracks it, will fix it at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants