-
Notifications
You must be signed in to change notification settings - Fork 26
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
Integrating AIR with the ROCm runtime #367
Conversation
d0ba712
to
7e2b322
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.
Other than the one question about libelf, everything else looks good
.github/workflows/buildAndTest.yml
Outdated
@@ -27,6 +27,27 @@ jobs: | |||
fetch-depth: 2 | |||
submodules: "true" | |||
|
|||
- name: Install necessary build tools for libelf |
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.
We recently decided to replace our custom libelf with the pre-built one. It means these tools are no longer necessary. I don't know if it's better to make that change in a different commit or to edit this one. What do you think?
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 can edit the commit. Thanks for the comments I see how you are doing it in your mlir-aie fork.
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.
In the moment felt better as a new commit so put in 04a27d4. Let me know what you think. I couldn't get CMake to automatically find LibElf if you know how to do that that could make it cleaner, but I am just pointing to the standard install location which also works.
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 know how to get CMake to search for a library. We could pass the path explicitly (like I was doing for the custom version) but with a default to the standard Ubuntu location. I think for now, relying on the standard Ubuntu location (as you are doing) is acceptable. If everyone will just use 'apt install' then it is highly unlikely the library will be at a different location.
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.
100% agree. Sweet will merge this in. Thanks!
.github/workflows/buildAndTest.yml
Outdated
- name: Rebuild and Install libxaie | ||
run: utils/github-clone-build-libxaie.sh | ||
|
||
- name: Rebuild and Install elfutils |
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.
Also part of the libelf change
.github/workflows/buildAndTest.yml
Outdated
@@ -86,7 +121,10 @@ jobs: | |||
-DMLIR_DIR=../llvm/install/lib/cmake/mlir/ \ | |||
-DLLVM_DIR=../llvm/install/lib/cmake/llvm/ \ | |||
-DAIE_DIR=`pwd`/../mlir-aie/install/lib/cmake/aie/ \ | |||
-DELFUTILS_DIR=$PWD/../elfutils \ |
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.
Also part of the libelf change
.github/workflows/buildAndTest.yml
Outdated
@@ -113,7 +151,10 @@ jobs: | |||
-DMLIR_DIR=../llvm/install/lib/cmake/mlir/ \ | |||
-DLLVM_DIR=../llvm/install/lib/cmake/llvm/ \ | |||
-DAIE_DIR=`pwd`/../mlir-aie/install/lib/cmake/aie/ \ | |||
-DELFUTILS_DIR=$PWD/../elfutils \ |
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.
Also part of the libelf change
CMakeLists.txt
Outdated
-DAIE_DIR=${AIE_DIR} | ||
-DELFUTILS_DIR=${ELFUTILS_DIR} |
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.
Also part of the libelf change
docs/building.md
Outdated
``` | ||
|
||
Next, clone and build MLIR-AIE with paths to llvm, aienginev2, and cmakeModules repositories. Again, we use a common installation directory. Note that in the following commands, we assume that the aienginev2 library is installed in /opt/xaiengine as directed in the `Building on x86 with runtime for PCIe` section. If the aienginev2 library was installed elsewhere, be sure that the 4th argument to build mlir-aie points to that location. | ||
Next, we are going to need to clone and build elfutils. This is used in the runtime to unpack and decode AIE application binaries. |
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.
Also part of the libelf change
docs/building.md
Outdated
``` | ||
./clone-mlir-aie.sh | ||
./build-mlir-aie-local.sh llvm mlir-aie/cmake/modulesXilinx /opt/xaiengine mlir-aie build ../../install | ||
./utils/github-clone-build-elfutils.sh |
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.
Also part of the libelf change
host.cpp | ||
pcie-ernic.cpp | ||
pcie-ernic-dev-mem-allocator.cpp | ||
network.cpp | ||
) | ||
set_property(TARGET airhost_shared PROPERTY POSITION_INDEPENDENT_CODE ON) | ||
|
||
add_library(libelf_pic STATIC IMPORTED) |
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.
Also part of the libelf change
utils/github-clone-build-elfutils.sh
Outdated
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.
Also part of the libelf change
Fixing tests to make them build correctly after rocm integration (#367).
No description provided.