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

add gemmx prameter support #327

Merged
merged 5 commits into from
Sep 17, 2024
Merged

add gemmx prameter support #327

merged 5 commits into from
Sep 17, 2024

Conversation

xiaoling-yi
Copy link
Collaborator

@xiaoling-yi xiaoling-yi commented Sep 16, 2024

In this PR, we add the parameter support for gemmx. More specifically, we:

  • set the gemmx pe array size, meshRow, tileSize, and meshCol as configurable parameters in the cfg file. Now we can set these parameters to different values, but please take care to modify the streamer parameters too! Just so you know, the transpose extension only works for 512 data width for readers.
  • Note: the pipelined SIMD only works for meshRow * meshCol = 64 cases so if you set pe array size that doesn't have 64 output, please take care to set with_pipeline = false.
  • modify software test and datagen.py to accommodate arbitrary pe array size. Now only fixed the matmul test. Conv test modification will be done later.
  • relevant modification in snax-gen.py. The format tool did some format, so it looks like many changes but only very minimal changes actually.

@xiaoling-yi xiaoling-yi marked this pull request as draft September 16, 2024 20:32
@xiaoling-yi xiaoling-yi marked this pull request as ready for review September 17, 2024 07:42
Copy link

@rgantonio rgantonio left a comment

Choose a reason for hiding this comment

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

OK! Would it be more useful though that there is a default setting for the GeMM config?

Otherwise, people who want to use your gemm needs to always set the size?

@xiaoling-yi
Copy link
Collaborator Author

xiaoling-yi commented Sep 17, 2024

Would it be more useful though that there is a default setting for the GeMM config?

I don't know. Maybe it's better to ensure that people explicitly set the PE size?
The streamer parameters should be matched with the PE array size

@rgantonio
Copy link

I don't know. Maybe it's better to ensure that people explicitly set the PE size?
The streamer parameters should be matched with the PE array size

I'm not too picky so okay haha. Maybe we can add somekind of documentation on our documentation page for this 😄

@xiaoling-yi
Copy link
Collaborator Author

I don't know. Maybe it's better to ensure that people explicitly set the PE size?
The streamer parameters should be matched with the PE array size

I'm not too picky so okay haha. Maybe we can add somekind of documentation on our documentation page for this 😄

We can!

Copy link

@JosseVanDelm JosseVanDelm left a comment

Choose a reason for hiding this comment

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

Hi, nice PR!

already a few high-level questions.
I'm not a big fan of our current build process because it routes everything to a single python command that can not be pipelined, and which parallel make does not like #228

For this specific PR though:

Just so you know, the transpose extension only works for 512 data width for readers.

Note: the pipelined SIMD only works for meshRow * meshCol = 64 cases so if you set pe array size that doesn't have 64 output, please take care to set with_pipeline = false.

Can you please please please verify this somewhere in software instead of just this comment

Who sets TRANSPOSE_EXTENSION_ENABLE in the software makefile?
Is there an automatic way this is set?

@xiaoling-yi
Copy link
Collaborator Author

For this specific PR though:

Just so you know, the transpose extension only works for 512 data width for readers.

Note: the pipelined SIMD only works for meshRow * meshCol = 64 cases so if you set pe array size that doesn't have 64 output, please take care to set with_pipeline = false.

Can you please please please verify this somewhere in software instead of just this comment

I verified locally. Later relevant documentation will be added to push a warning on this.

Who sets TRANSPOSE_EXTENSION_ENABLE in the software makefile? Is there an automatic way this is set?

The streamer header file gen will set it if a transpose extension is instantiated.

@xiaoling-yi xiaoling-yi merged commit 4fae969 into main Sep 17, 2024
23 checks passed
@xiaoling-yi xiaoling-yi deleted the xyi/add-para-gemmx branch September 18, 2024 10:43
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.

3 participants