Switch aie-rt submodule from personal fork to upstream + vendored patches#3334
Draft
jgmelber wants to merge 1 commit into
Draft
Switch aie-rt submodule from personal fork to upstream + vendored patches#3334jgmelber wants to merge 1 commit into
jgmelber wants to merge 1 commit into
Conversation
…ches third_party/aie-rt tracked stephenneuendorffer/aie-rt (a personal fork with no review process) instead of upstream Xilinx/aie-rt. Point the submodule at upstream, pinned at the exact merge-base with the fork so this is a pure de-fork with no behavior change, and vendor the functionality mlir-aie actually needs that isn't upstream yet (VCK5000/AMDAIR IO backend used by runtime_lib/test_lib/test_library.cpp, cdo_rts.h/main_rts.h Vitis-header workarounds, and a resource-manager leak fix) as a patch file under third_party/patches/aie-rt, applied automatically at CMake configure time. Verified the patched tree is byte-identical to the previous fork-pinned tree, and that all three build paths that consume third_party/aie-rt (the xaiengine ExternalProject, the in-tree xaienginecdo_static target, and test_lib) build correctly from a freshly-unpatched submodule checkout. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jgmelber
requested review from
abisca,
fifield,
jackl-xilinx and
stephenneuendorffer
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR de-forks the third_party/aie-rt submodule by pointing it at upstream Xilinx/aie-rt, and preserves required downstream-only behavior by vendoring the fork delta as an in-tree patch series that is applied automatically during CMake configure.
Changes:
- Switched the
third_party/aie-rtsubmodule URL from a personal fork to upstream (.gitmodules). - Added a vendored patch (
third_party/patches/aie-rt/0001-...) plus documentation for the downstream delta. - Integrated automatic patch application into both the in-tree runtime build and the standalone xaiengine build paths.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
third_party/patches/aie-rt/README.md |
Documents the purpose and contents of vendored aie-rt patches. |
third_party/patches/aie-rt/0001-vck5000-cdo-sim-fixes.patch |
Introduces the downstream aie-rt delta as a git apply-able patch (AMDAIR backend, sim/CDO header workarounds, leak fix, etc.). |
runtime_lib/xaiengine/lib/CMakeLists.txt |
Applies vendored aie-rt patches before building xaiengine from the submodule. |
runtime_lib/xaiengine/aiert.cmake |
Adds the patch-application function invoked by multiple build entry points. |
runtime_lib/CMakeLists.txt |
Ensures patches are applied early enough for in-tree targets that glob aie-rt sources at configure time. |
.gitmodules |
Updates the aie-rt submodule to track upstream instead of a personal fork/branch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+17
to
+18
| +/* Suppress warning of unused parameter */ | ||
| +#define UNUSED(_x) (_x = _x) |
Comment on lines
+77
to
+83
| @@ -103,6 +103,7 @@ typedef enum { | ||
| XAIE_IO_BACKEND_IPU, /* IPU Backend */ | ||
| XAIE_IO_BACKEND_SOCKET, /* Socket backend */ | ||
| XAIE_IO_BACKEND_CONTROLCODE, | ||
| + XAIE_IO_BACKEND_AMDAIR, /* Use with amdair driver */ | ||
| XAIE_IO_BACKEND_MAX | ||
| } XAie_BackendType; |
| * | ||
| *******************************************************************************/ | ||
| -#define XAie_InstDeclare(Inst, ConfigPtr) XAie_DevInst Inst = { 0 } | ||
| +#define XAie_InstDeclare(Inst, ConfigPtr) XAie_DevInst Inst = {} |
Comment on lines
+880
to
+883
| +#include <unistd.h> | ||
| + | ||
| +typedef unsigned int uint; | ||
| + |
Comment on lines
+8
to
+31
| function(apply_aie_rt_vendor_patches AIE_RT_ROOT PATCH_DIR) | ||
| set(_sentinel ${AIE_RT_ROOT}/driver/src/io_backend/ext/xaie_amdair.c) | ||
| if(EXISTS ${_sentinel}) | ||
| return() | ||
| endif() | ||
|
|
||
| file(GLOB _patches ${PATCH_DIR}/*.patch) | ||
| list(SORT _patches) | ||
| find_package(Git REQUIRED) | ||
| foreach(_patch ${_patches}) | ||
| message(STATUS "Applying vendored aie-rt patch: ${_patch}") | ||
| execute_process( | ||
| COMMAND ${GIT_EXECUTABLE} apply ${_patch} | ||
| WORKING_DIRECTORY ${AIE_RT_ROOT} | ||
| RESULT_VARIABLE _patch_result | ||
| ERROR_VARIABLE _patch_error) | ||
| if(NOT _patch_result EQUAL 0) | ||
| message(FATAL_ERROR | ||
| "Failed to apply vendored aie-rt patch ${_patch}:\n${_patch_error}\n" | ||
| "If ${AIE_RT_ROOT} has local modifications, reset it with " | ||
| "'git -C ${AIE_RT_ROOT} checkout -- .' and re-run CMake.") | ||
| endif() | ||
| endforeach() | ||
| endfunction() |
Comment on lines
+304
to
+306
| + read(value, buf, strlen(buf) + 1); | ||
| + close(value); | ||
| + *Data = strtoul(buf, 0, 0); |
Comment on lines
+497
to
+506
| + UNUSED(IOInst); | ||
| + UNUSED(RegOff); | ||
| + UNUSED(Mask); | ||
| + UNUSED(Value); | ||
| + UNUSED(TimeOutUs); | ||
| + | ||
| + XAIE_DBG("NPIMP: 0x%lx, 0x%x, 0x%x, 0x%d\n", AmdAirIOInst->NpiBaseAddr + RegOff, | ||
| + Mask, Value, TimeOutUs); | ||
| + | ||
| + return XAIE_OK; |
Comment on lines
+209
to
+213
| + // recover sysfs path | ||
| + if (!DevInst->IOInst) { | ||
| + XAIE_ERROR("sysfs path not present!\n"); | ||
| + return XAIE_ERR; | ||
| + } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
third_party/aie-rttrackedstephenneuendorffer/aie-rt(a personal fork with no review process) on branchstrix_2024.2, pinned at6a15e48b. Point it at upstreamXilinx/aie-rtinstead, pinned ate2aca220— the exact merge-base with the fork — so this is a pure de-fork with zero behavior/version drift.third_party/patches/aie-rt/0001-vck5000-cdo-sim-fixes.patch, applied automatically at CMake configure time:xaie_amdair.c), actively used byruntime_lib/test_lib/test_library.cppfor VCK5000 hardware testing — not dead code, and not present anywhere upstream.cdo_rts.h/main_rts.hworkarounds avoiding Vitis-only header dependencies.RscArrPerTile).third_party/aie-rt: the standaloneruntime_lib/xaiengine(used by thexaiengine_<target>ExternalProject) and the in-treexaienginecdo_statictarget inruntime_lib/CMakeLists.txt, which previously globbed the submodule directly at parent-configure time.Why not other approaches
Test plan
e2aca220and reproduces a tree byte-identical to the previous fork-pinned tree (aside fromMakefile.Linux, which mlir-aie's CMake build doesn't use).runtime_lib/xaienginestandalone (thexaienginecdoshared lib) from a clean, unpatched submodule checkout — patch auto-applies once at configure, is a no-op on reconfigure, and the resulting.soexportsAmdAirBackend.xaiengine_x86_64ExternalProject, the in-treexaienginecdo_statictarget, andtest_lib(which referencesXAIE_IO_BACKEND_AMDAIR) — all build, link, and install successfully.🤖 Generated with Claude Code