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

Modify aie2xclbin tool to allow merging kernels in existing XCLBIN #1561

Merged
merged 2 commits into from
Jun 15, 2024

Conversation

nirvedhmeshram
Copy link
Collaborator

This adds the same feature added on the python side by #1508
Tested with iree-amd-aie which as far as I know is the only client of this tool.
I am also assuming that iree-amd-aie will correctly provide unique kernel ids here as the tool doesn't give any warnings if you merge two kernels of same kernel id but then gives incorrect results at runtime.
Here is a WIP PR of how iree-amd-aie will use this. Will clean that PR once we have a wheel from this one.
nod-ai/iree-amd-aie#418

Copy link
Collaborator

@newling newling left a comment

Choose a reason for hiding this comment

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

xclbinutil is part of XRT AFAIK. Before this PR, could we generate vmfbs without xrt? The instructions here: https://github.com/nod-ai/iree-amd-aie?tab=readme-ov-file#runtime-driver-setup explain how to build iree-amd-aie with runtime support, sounds like XRT is only needed for if you want to runtime support there. But those instructions don't seem complete anyway, as there is no mention of building mlir-aie.

It'd be nice one day to have a CI to check the "no runtime" workflow too. Anyway, my overarching thought/question is that it's quite nice for developers to work on codegen without XRT. Hopefully this PR doesn't change that.

My only other nitty comment is that maybe it would be better to pass the path to xrt directly when looking for xclbinutil, instead of using auto xclbinutil = sys::findProgramByName("xclbinutil"), like is done for peano (i.e. there is a std::string PeanoDir) (not requesting a change though)

@fifield
Copy link
Collaborator

fifield commented Jun 15, 2024

xclbinutil is part of XRT AFAIK. Before this PR, could we generate vmfbs without xrt? The instructions here: https://github.com/nod-ai/iree-amd-aie?tab=readme-ov-file#runtime-driver-setup explain how to build iree-amd-aie with runtime support, sounds like XRT is only needed for if you want to runtime support there. But those instructions don't seem complete anyway, as there is no mention of building mlir-aie.

It'd be nice one day to have a CI to check the "no runtime" workflow too. Anyway, my overarching thought/question is that it's quite nice for developers to work on codegen without XRT. Hopefully this PR doesn't change that.

This PR does not change the xclbinutil requirement. It is already required to generate an xclbin.

@nirvedhmeshram
Copy link
Collaborator Author

About changing the way it finds xclbinutil, the only way I see that working is adding an extra flag to iree-amd-aie for the XRT path, which I feel will make the iree-compile command further cluttered. Anyway since this point is not directly related to this PR I will land this so I have wheels on Monday to work on the iree-amd-aie side PR. Thanks for the comments.

@nirvedhmeshram nirvedhmeshram added this pull request to the merge queue Jun 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 15, 2024
@nirvedhmeshram nirvedhmeshram added this pull request to the merge queue Jun 15, 2024
Merged via the queue into Xilinx:main with commit 18c8815 Jun 15, 2024
51 checks passed
@nirvedhmeshram nirvedhmeshram deleted the nm_xclbin_merge branch June 15, 2024 20:05
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