-
Notifications
You must be signed in to change notification settings - Fork 166
IRON host runtime abstraction #2737
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
Conversation
|
This is great. I'm wondering about the layering. Could non-IRON mlir-aie users like mlir-air use this without importing all of iron? |
|
@fifield good question. I think it's tied to IRON unless we moved some of this logic to pyxrt. However, installing the mlir-aie wheels is maybe not too bad anymore? |
|
@ypapadop-amd Thanks for the review! My plan had been to do a separate PR for documentation since docs were not built at all in utils, but there's no time like the present, so I went ahead and fixed up and added doc strings and added the hostruntime-related files in python/utils to the generated API documentation. |
| @@ -7,8 +7,9 @@ | |||
| # (c) Copyright 2024 Advanced Micro Devices, Inc. or its affiliates | |||
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.
Copyright needs to be updated to a range in those.
|
I saw that the set_current_device test was removed. Is the function removed too? For some kernels, that's the only way to set the device. |
|
@ypapadop-amd my intent is that:
The full realization of this plan would only happen after further improvements to the JIT/compilation code, which this PR does not do. So my question is:
|
Is now the default to pass the device type in the kernel arguments? Would it work for example with mlir-aie/programming_examples/basic/vector_vector_add/vector_vector_add_placed.py Line 51 in 5b52c47
Probably, I use set_current_device / get_current_device to pass that information.
I am not sure what the long-term plan is. I thought that for JIT there's always an implicit device (makes sense) and an aversion to passing it as an argument (thus the set_current_device / get_current_device). Are we going to separate the JIT kernel entrypoint from the implementation? I.e., something like: def kernel_impl(device):
...
return Program(device, rt).resolve_program(SequentialPlacer())
@iron.jit
def kernel(...)
return kernel_impl(get_current_device()) |
|
Yes, the eventual goal is to separate compilation from the JIT-running of the module. So even something that was jit decorated could be compiled with a custom set device. For cross compilation, there SHOULD be an argument which is the device type. So I would not say I have an aversion to passing the device as arguments. With the current PR, you should be able to customize the behavior of iron.DefaultNPURuntime = <some custom python class the returns your device of choice>
iron.get_current_device() # this should now return your device of choiceThis is admittedly clunky but this PR is already huge, and I won't want to step more into making changes to JIT in it than I need to. |
|
I think that's undesirable for user code. Can we have a dummy runtime checked in as part of this PR that does that? |
|
It is undesirable. If you want to create a dummy host runtime or other host runtime code to meet additional needs beyond what is used/tested in this repo, I think that would be an excellent follow on PR for you to make :) |
|
@ypapadop-amd I think the changes requested originally are satisfied, can you remove the request changes please? |
|
@ypapadop-amd I re-added |
Summary
This PR begins the process of consolidating runtime support code into one implementation that supports tracing, JIT, programming examples, and tests.
Apologies that the PR is so large, but many components of the repo were good candidates to use shared runtime library functions!
Details
The primary goals of this work are to:
AIE_Applicationclass in theaie.utils.xrtmodule) and the amd/IRON runtime code (e..g, the AIEDeviceManager class in https://github.com/amd/IRON/blob/devel/applications/llama_3.2_1b/src/aie_device_manager.py). This will help maintainability and ensure improvements made for efficiency (e.g., in pre-loading, buffer handling, etc.) are consistently used.iron.Tensors.Other miscellaneous things included in this PR:
test_*.pynaming convention. Make sure lit test files are not named with this convention. This allows one to runpytest test/pythonand successfully run all the pytests in that directory. This runs locally much faster than trying to use lit.TraceConfigobject. Hopefully this will complement and not conflict with [proposal][wip] Add event trace configuration to mlir-aie #2705What this PR does NOT do: