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 amd-aie-direct HAL target (3/n) #420

Merged
merged 3 commits into from
Jun 24, 2024

Conversation

makslevental
Copy link
Collaborator

@makslevental makslevental commented Jun 16, 2024

This PR is part of a stack of PRs that refactor the dependency on MLIR-AIE. See #430 for more information.

This PR adds a new target amd-aiet-direct, directly references AIEVec passes (for eventual lowering of core code to chess LLVM IR) and also (simultaneously) vendors XCLBinGen.cpp from mlir-aie (second part used to be broken out in #422 but on @newling's suggestion I squashed). Note, XCLBinGen and AIETargetDirect are basically carbon copies of what currently exists except AIETargetDirect doesn't shell out and calls aie2xclbin directly. Note also that XCLBinGen will undergo further revision up to for the chess backend (but that'll have to wait until I get home).

To test, I added some lines to print_ir_aie2xclbin.sh. I'm happy to bike shed this but probably not too hard.

@makslevental makslevental changed the base branch from makslevental/remove-mlir-aie/aie_r_dep to makslevental/remove-mlir-aie/map_object_fifo_to_lib June 16, 2024 20:07
@makslevental makslevental changed the title Add amd-aie-direct HAL target (2/n) Add amd-aie-direct HAL target (3/n) Jun 16, 2024
@makslevental makslevental force-pushed the makslevental/remove-mlir-aie/direct_target branch from 7d93c21 to 5a46312 Compare June 16, 2024 20:27
@makslevental makslevental force-pushed the makslevental/remove-mlir-aie/map_object_fifo_to_lib branch from 8e65276 to 9b3416a Compare June 16, 2024 20:28
@makslevental makslevental force-pushed the makslevental/remove-mlir-aie/direct_target branch 2 times, most recently from dfcbfa3 to a295d90 Compare June 16, 2024 21:14
@makslevental makslevental linked an issue Jun 17, 2024 that may be closed by this pull request
@makslevental makslevental force-pushed the makslevental/remove-mlir-aie/direct_target branch from a295d90 to 41c9b5e Compare June 18, 2024 04:50
@makslevental makslevental force-pushed the makslevental/remove-mlir-aie/map_object_fifo_to_lib branch 2 times, most recently from 77bead4 to 775351e Compare June 19, 2024 20:51
@makslevental makslevental force-pushed the makslevental/remove-mlir-aie/direct_target branch from 41c9b5e to 37599a1 Compare June 19, 2024 20:51
@makslevental makslevental force-pushed the makslevental/remove-mlir-aie/map_object_fifo_to_lib branch 5 times, most recently from b375fab to dfea3a6 Compare June 19, 2024 22:27
@makslevental makslevental force-pushed the makslevental/remove-mlir-aie/direct_target branch 4 times, most recently from 7fa1376 to d4dea46 Compare June 19, 2024 23:08
@makslevental makslevental force-pushed the makslevental/remove-mlir-aie/map_object_fifo_to_lib branch 3 times, most recently from 329e0e7 to 2eda0d0 Compare June 20, 2024 15:37
Base automatically changed from makslevental/remove-mlir-aie/map_object_fifo_to_lib to main June 20, 2024 15:47
@makslevental makslevental force-pushed the makslevental/remove-mlir-aie/direct_target branch from d4dea46 to 7c45ac3 Compare June 20, 2024 16:38
@makslevental makslevental force-pushed the makslevental/remove-mlir-aie/direct_target branch 15 times, most recently from bf23c05 to 3578f9e Compare June 22, 2024 17:49
@makslevental
Copy link
Collaborator Author

@newling @nirvedhmeshram this is now ready for your keen eyes!

@makslevental makslevental force-pushed the makslevental/remove-mlir-aie/direct_target branch from 3578f9e to ac58e66 Compare June 22, 2024 18:03
Copy link
Contributor

@nirvedhmeshram nirvedhmeshram left a comment

Choose a reason for hiding this comment

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

Thanks, looks good, just some minor comments.

Copy link
Contributor

@nirvedhmeshram nirvedhmeshram left a comment

Choose a reason for hiding this comment

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

LGTM, after fixing the test/CI.

Copy link
Contributor

@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.

Some minor comments but they can all be ignored as I think it'll easier to make changes once everything is landed, and temporary code is removed.

std::string XCLBinKernelID;
std::string XCLBinInstanceName;
bool UseChess = false;
bool DisableThreading = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

These flags -- do we still need them?

Specifically 'DisableThreading' here is just something we had to throw over the wall because the mlir-aie passes are run on a completely new process (i.e. a new MLIR Context). If the passes are now being run in the same context.

Not a request for change, just some information for you.


namespace mlir::iree_compiler::AMDAIE {

// static llvm::cl::opt<std::string> clEnableAMDAIEUkernels(
Copy link
Contributor

Choose a reason for hiding this comment

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

Old comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This flag is in AIETarget and so this target should also support it but I can't register the cl::opt (because they're static globals). So I left it here as a reminder to myself.

@makslevental makslevental force-pushed the makslevental/remove-mlir-aie/direct_target branch 2 times, most recently from 52d7a11 to d4a34e1 Compare June 24, 2024 21:37
@makslevental makslevental force-pushed the makslevental/remove-mlir-aie/direct_target branch from d4a34e1 to 3f3cd29 Compare June 24, 2024 21:39
@makslevental makslevental merged commit 2b14446 into main Jun 24, 2024
2 checks passed
@makslevental makslevental deleted the makslevental/remove-mlir-aie/direct_target branch June 24, 2024 22:15
nirvedhmeshram added a commit that referenced this pull request Jun 28, 2024
Since #420 added a internal
utility to go from AIE to XCLBIN it makes sense for all flows to use
that.
This utility doesn't add any AIE lowering passes so the
`TranslationPassPipeline`s are responsible for that. Hence the passes
needed for the IR lowered through the AIR flow to be further lowered to
AIE are added in its lowering pipeline.

There were some minor additions needed to the utility to get it working
e2e for the AIE flow.

We no longer need to have a separate `aie2xclbin-disable-threading` as
we are passing the existing context and the `mlir-disable-threading`
flag gets automatically propagated in this case. And it actually gives
an error to try changing it if the passed context is multi-threaded.

With this PR the AIE lowering is becoming a part of the IREE
`TranslationPassPipeline` so the pack-peel lit test that was just
passing the air-to-aie conversion had to be watered down to reflect what
can actually be lowered to AIE.

Unfortunately, we still also need the mlir-aie wheel as the internal
utility is still expecting a mlir-aie install for me_basic.o library
[here](https://github.com/nod-ai/iree-amd-aie/blob/main/compiler/plugins/target/AMD-AIE/iree-amd-aie/Target/XCLBinGen.cpp#L302-L304)
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.

[Tracking] Refactor MLIR-AIE dependency
3 participants