Skip to content

Cores per column placer 2#2619

Merged
pvasireddy-amd merged 18 commits intoXilinx:mainfrom
kurtis-b-1:cores-per-column-placer-2
Oct 7, 2025
Merged

Cores per column placer 2#2619
pvasireddy-amd merged 18 commits intoXilinx:mainfrom
kurtis-b-1:cores-per-column-placer-2

Conversation

@kurtis-b-1
Copy link
Copy Markdown
Contributor

Building on top of branch: https://github.com/Xilinx/mlir-aie/tree/cores-per-column-placer

The "column-wise" unplaced design now maps the reduction across the columns and at the final column performs reduction across the rows using the column-limited placer. It should be possible to use any number of cores up to 8 for the Phoenix and 16 for the Strix.

Renamed the original designs to "row-wise" designs, which use the mapping from the sequential placer, and kept them the same as before.

Copy link
Copy Markdown
Contributor

@AndraBisca AndraBisca left a comment

Choose a reason for hiding this comment

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

The example in this PR looks good. More comments in the Column-Limited placer and just the placers.py in general would be good. How much code duplication is there between this new placer and the Sequential one?

Comment thread programming_examples/basic/vector_reduce_max/multi_column_designs/README.md Outdated
Comment thread python/iron/placers.py Outdated
Comment thread python/iron/placers.py Outdated
Copy link
Copy Markdown
Collaborator

@fifield fifield left a comment

Choose a reason for hiding this comment

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

Please add some sort of unit test for the new placer functionality...some small snippets of iron as input, then check the mlir is correct in the outputs. I don't see any such tests added in this or the cores-per-column-placer branch, and I'm not sure if they exist at all for the placer.

Of course what I'd really like is for this functionality to migrate into the core mlir-aie infrastructure, e.g. #2265, but so far nobody is interested in that other than me.

@andrej
Copy link
Copy Markdown
Collaborator

andrej commented Sep 30, 2025

Of course what I'd really like is for this functionality to migrate into the core mlir-aie infrastructure, e.g. #2265, but so far nobody is interested in that other than me.

I am also interested in that and more generally migrating some of the higher-level IRON abstractions into MLIR. I believe @jgmelber expressed to me before that he thinks this would be good too.

@kurtis-b-1
Copy link
Copy Markdown
Contributor Author

Please add some sort of unit test for the new placer functionality...some small snippets of iron as input, then check the mlir is correct in the outputs.

I added a test that checks the mapping with Column-Limited placer set to 2 cores per column. Please let me know if I should modify it or if it doesn't make sense.

@kurtis-b-1 kurtis-b-1 requested a review from fifield October 1, 2025 16:24
@AndraBisca
Copy link
Copy Markdown
Contributor

Thank you for making the changes @kurtis-b-1! I have one last nitpick about the naming, which currently makes me think that this placer limits the number of columns. Maybe something like ComputeLimitedPlacer would be more descriptive?

Comment thread python/iron/placers.py Outdated
Comment thread programming_examples/basic/vector_reduce_max/multi_column_designs/README.md Outdated
Comment thread programming_examples/basic/vector_reduce_max/multi_column_designs/README.md Outdated
@kurtis-b-1
Copy link
Copy Markdown
Contributor Author

@AndraBisca Thanks for catching those! Updated the documentation as per the comments.

@pvasireddy-amd pvasireddy-amd added this pull request to the merge queue Oct 7, 2025
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 7, 2025
@pvasireddy-amd pvasireddy-amd added this pull request to the merge queue Oct 7, 2025
Merged via the queue into Xilinx:main with commit c146f8c Oct 7, 2025
56 of 57 checks passed
@kurtis-b-1 kurtis-b-1 deleted the cores-per-column-placer-2 branch November 4, 2025 17:23
fifield pushed a commit to fifield/mlir-aie that referenced this pull request Nov 12, 2025
Co-authored-by: Pranathi Vasireddy <pvasired@amd.com>
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