-
Notifications
You must be signed in to change notification settings - Fork 29
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
Enable initial executable linking + mlir-air bump #395
Conversation
183bbd8
to
fd2a7ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have much context, but the codes generally LGTM. Is this working currently? It's good to add a test.
compiler/plugins/target/AMD-AIE/iree-amd-aie/Target/AIETarget.cpp
Outdated
Show resolved
Hide resolved
Not sure what happened in CI, looks like an xclbin was created but then the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This .fbs stuff is vaguely familiar but I also feel like I'm missing context, so my comments are all very shallow. Looks fine though
compiler/plugins/target/AMD-AIE/iree-amd-aie/Target/AIETarget.cpp
Outdated
Show resolved
Hide resolved
compiler/plugins/target/AMD-AIE/iree-amd-aie/Target/AIETarget.cpp
Outdated
Show resolved
Hide resolved
Yes I wanted to add a e2e test but couldnt find a easy way to do so with the current CI infrastructure here is a example matmul sequence I tested locally
The main blocker is that the signing process needs to account for the new directory structure that happens after the linking optimization. I am not too inclined to fix it becuase once we update the driver we wont have to sign anything so maybe we can add a test after that? In the mean time I have opened this issue |
It was because the directory structure / file names are different with the link optimization, I changed that if there is only one dispatch. Lets see if that helps. |
748efc2
to
d2c9a7c
Compare
@newling could you PTAL at the new CI side changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! As you are bumping AIR in this PR, can you bump it to the latest again? Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you remove build_tools/ci/rm_xclbin.sh can you please update the test cpu_comparison/run_test.sh
too?
I already did? Is something missing in it? |
c305e4a
to
920873f
Compare
Adds the pass to link multiple HAL executables together to the pass pipeline. The resulting linke3d executable can have several entry points. At artifact creation time we create artifact for each entry point one at a time by passing the corresponding aie.device op to the
aie2xclbin
tool. In a later PR we will merge the xclbins when possible.The executable schema is updated to account for flexibility needed to have shared xclbins / lx6 instruction streams.
We also add logic to use assigned ordinals of entry points rather than assuming ascending order as noted by iree-org/iree#15905
An mlir-air bump is needed for this to work e2e.
Progress towards: #380