feat(iron): inline external kernels into the core (no func.call, no separate .o) - #3399
Open
fifield wants to merge 21 commits into
Open
feat(iron): inline external kernels into the core (no func.call, no separate .o)#3399fifield wants to merge 21 commits into
fifield wants to merge 21 commits into
Conversation
fifield
force-pushed
the
inline-external-funcs
branch
4 times, most recently
from
July 28, 2026 16:51
65520dc to
180ecb8
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in “inline external kernel” path for IRON ExternalFunction kernels by compiling C++ kernels to LLVM IR (.ll/.bc) and having aiecc merge them into the core’s LLVM module via llvm-link (Peano-only), enabling the always-inliner to eliminate the func.call boundary and avoid separately linking kernel objects.
Changes:
- Add collection/routing of
.ll/.bc“IR link artifacts” soaiecccanllvm-linkthem into the core module on the Peano path, while rejecting them early on the Chess path. - Teach ldscript/BCF emitters to skip
.ll/.bcin object-link lists to avoid invalid linker inputs / duplicate definitions. - Add Python support for
ExternalFunction(inline=True)andcompile_cxx_core_function(inline=True)plus tests and a microbenchmark example.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/aiecc/IRTransforms.h | Add helpers to collect per-core/per-device .ll/.bc link artifacts (absolute path + dedup). |
| tools/aiecc/aiecc.cpp | Add an IR-merge path (llvm-link) for Peano; reject IR artifacts in Chess flow; plumb IR link lists through graph. |
| test/python/test_kernels_memoization.py | Add coverage for digest-based disambiguation preserving .ll suffix for inline artifacts. |
| test/python/test_kernels_chess.py | Add test ensuring inline=True is rejected when using Chess. |
| test/python/test_inline_ir_rewrite.py | New unit tests for _make_ir_inlinable rewrite correctness across LLVM IR grammar cases. |
| test/python/npu-xrt/test_jit_extern_functions_inline.py | New end-to-end NPU test that inline path matches object-linked results. |
| test/python/npu-xrt/test_compile_link.py | Add API signature regression test and inline .ll/.bc compile checks. |
| test/python/external_func_inline_link_with.py | IR-only checks that inline kernels declare .ll in link_with and normalize explicit names. |
| test/aiecc/cpp_link_with_ir_artifacts.mlir | Verify aie-assign-core-link-files keeps all artifacts and emitters filter out .ll/.bc. |
| test/aiecc/cpp_link_with_ir_artifacts_fallback.mlir | Verify deprecated core-level link_with fallback also filters out .ll. |
| python/utils/compile/utils.py | Implement compile_cxx_core_function(inline=...) and LLVM-IR rewriting (_make_ir_inlinable); reject unsupported combos during compilation. |
| python/iron/kernel.py | Add ExternalFunction(inline=...) surface API and filename normalization for .ll/.bc. |
| programming_examples/basic/inline_kernel/run.lit | Add lit runner for NPU1 microbenchmark/correctness example. |
| programming_examples/basic/inline_kernel/run_strix.lit | Add lit runner for NPU2 (Strix) microbenchmark/correctness example. |
| programming_examples/basic/inline_kernel/README.md | Document inline mode, constraints, and performance motivation. |
| programming_examples/basic/inline_kernel/inline_kernel.py | Add microbenchmark comparing object-linked vs inlined external kernel calls. |
| lib/Targets/AIETargetLdScript.cpp | Skip .ll/.bc artifacts when emitting INPUT(...) entries. |
| lib/Targets/AIETargetBCF.cpp | Skip .ll/.bc artifacts when emitting _include _file entries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fifield
marked this pull request as ready for review
July 28, 2026 17:50
fifield
requested review from
abisca,
andrej,
denolf,
erwei-xilinx,
hunhoffe,
jackl-xilinx,
jgmelber and
stephenneuendorffer
as code owners
July 28, 2026 17:50
erwei-xilinx
approved these changes
Jul 28, 2026
hunhoffe
reviewed
Jul 28, 2026
hunhoffe
approved these changes
Jul 28, 2026
hunhoffe
left a comment
hunhoffe
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for getting this ready to go!!
Might be nice to use some of the newer Python utils in the example, but that's just a nice-to-have.
…eparate .o) External function calls dominate tight IRON loops: control stays in Python while kernels are C++, so a core issues a func.call per tile (issue Xilinx#3396). Add an opt-in inline path that folds the kernel body directly into the aie.core. ExternalFunction(inline=True) / compile_cxx_core_function(inline=True): compile the C++ kernel to alwaysinline + linkonce_odr LLVM IR (.ll) via clang -S -emit-llvm instead of a .o. The func.func declaration carries a .ll link_with. aiecc: link_files entries ending in .ll/.bc are routed to an IR-merge list, llvm-link'd into the core LLVM module before opt/llc, and excluded from object linking (ld-script INPUT() / BCF _include) so each symbol is merged once. The always-inliner folds the alwaysinline body in; linkonce_odr lets globaldce drop the now-dead definition. Per-core and unified paths; Peano only (Chess cannot llvm-link). inline + symbol_prefix is rejected: an inline kernel is emitted as LLVM IR and cannot be symbol-renamed via llvm-objcopy. Tests/bench: - test/python/external_func_inline_link_with.py: IR-only check that inline=True declares a .ll link_with (no hardware). - test/python/npu-xrt/test_jit_extern_functions_inline.py: end-to-end on NPU; the inline result matches the object-linked path. - programming_examples/basic/inline_kernel: object-vs-inline latency microbench. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
_make_ir_inlinable anchored its insertion on the trailing `{`, which puts
the attribute after any clause the grammar places between [fn attrs] and
the body. Real Peano output hits this:
-g -> `... #0 !dbg !7 alwaysinline {` llvm-as: parse error
-O0 -> alwaysinline + optnone llvm-as: verifier error
Both are reachable through ExternalFunction(compile_flags=...).
Insert into the [fn attrs] slot instead: paren-match the parameter list
(so nested byval(...)/sret({...}) and quoted attribute strings are
handled), skip an optional unnamed_addr/addrspace, and insert there.
Three adjacent bugs in the same function, fixed with it:
* Linkage was guarded by `"linkonce_odr" not in line`, so an existing
linkage keyword produced `define linkonce_odr internal ...` -- two
linkages is a parse error. Check the token after `define` against the
linkage keyword set; replace `external`, leave already-discardable
linkages alone.
* alwaysinline cannot coexist with noinline/optnone, which arrive via a
shared `attributes #N` group. Repoint the define at a private copy with
those dropped rather than mutating a group other functions reference.
* A mangled (non-`extern "C"`) kernel silently did not inline; it now
fails with the reason.
Verified against the Peano toolchain: the example kernel compiled at
default / -g / -O0 / -O0 -g / -fexceptions is accepted by llvm-as and opt
in all five configurations.
test_inline_ir_rewrite.py covers seven define shapes, linkage handling,
the attribute-group copy, idempotency and symbol matching, plus an
llvm-as parse check per shape when Peano is available. It needs no
hardware, and 18 of its 26 cases fail against the previous
implementation.
Also applies black to the .bc error-handling block, which was already on
the branch and not format-clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The BCF emitter skips .ll/.bc link files unconditionally, because on the peano path they are merged into the core module by llvm-link instead of being object-linked. buildObjectSubgraph's chess branch never consumed irLinkFiles, so under --xchesscc those artifacts were dropped on both routes at once and the kernel simply vanished -- surfacing as an undefined symbol from the chess linker, well away from the cause. IRON already rejects inline=True + use_chess, but aiecc also takes hand-written and third-party MLIR, so guard it here too: bundle irLinkFiles into the chess path and fail with the offending link_with entries listed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The emitter half of the inline path had no non-hardware coverage: the only thing exercising it was test_jit_extern_functions_inline.py, gated on %run_on_npu1/2% plus xrt_python_bindings. On a machine without an NPU nothing checked that .ll/.bc stay out of INPUT() / `_include _file`. Two tests alongside the existing cpp_link_with_* files: * cpp_link_with_ir_artifacts.mlir -- canonical func.func path. Asserts aie-assign-core-link-files records all three artifacts unfiltered (routing is the emitters' job), and that only the .o reaches the linker. * cpp_link_with_ir_artifacts_fallback.mlir -- the deprecated core-level link_with branch, which filters separately and so needs its own case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
inline_kernel.py carried `# RUN:` / `# CHECK: PASS!` in its header, but programming_examples/lit.cfg.py and basic/lit.local.cfg both set config.suffixes = ['.lit'], so those directives were never collected -- the example did not run anywhere, and it shipped no run*.lit unlike every sibling. A regression in the inline path would have gone unnoticed. Add run.lit (npu1) and run_strix.lit (npu2) following basic/transposes/run_jit.lit, and drop the inert header -- it was the only example .py in the tree carrying RUN lines. REQUIRES gains `peano`, since inline=True cannot use the Chess front-end. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
f9ef14b moved the inline + symbol_prefix guard into __init__, but the suggestion replaced the three assignments that used to sit there instead of adding the guard beside them: - self._original_name = name - self._symbol_prefix = symbol_prefix - self._inline = inline + if inline and symbol_prefix: + raise NotImplementedError(...) _instances.add(self) at the end of __init__ hashes the instance, and __hash__ -> _content_digest() reads self._inline, so *every* ExternalFunction construction raised AttributeError -- not only inline ones. test_kernels_specs.py, test_symbol_prefix.py and test_kernels_memoization.py all fail on it. Restore the assignments, keeping the relocated guard, and note the ordering constraint so the block is not moved below the collision scan again. _original_name and _symbol_prefix have the same requirement from compile_external_kernel (source file naming and the objcopy symbol rename). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pyright: "params_at is possibly unbound" (reportPossiblyUnboundVariable). It was assigned only inside the `if match:` branch of the define scan, and pyright cannot tie that to the `define_idx < 0` guard that follows. Not reachable at runtime -- the guard raises before params_at is read -- but it fails the type-check job. Initialize it alongside define_idx. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test_compile_inline_ir[.bc] failed on the Ryzen AI runner:
RuntimeError: Peano llvm-as not found in
.../site-packages/llvm-aie/bin/llvm-as
The llvm-aie wheel ships clang++, llvm-link, opt and llc but not
llvm-as, so the .bc branch could never work on a wheel-based install --
only on a full Peano source build. The [.ll] case and the end-to-end
inline test passed in the same run, confirming llvm-link is present and
llvm-as simply is not.
Assemble with clang++ instead. It is already a hard requirement of this
function, so no new dependency:
clang++ -x ir - -c -emit-llvm -O0 -Xclang -disable-llvm-passes
-disable-llvm-passes keeps it a pure text->bitcode conversion. clang
normalizes `alwaysinline` into the function's attribute group instead of
leaving it on the `define`; llvm-link and the always-inliner honor both
spellings, and a round-trip through llvm-dis confirms both alwaysinline
and linkonce_odr survive.
Verified against a bin/ directory mirroring the wheel (clang++,
llvm-link, opt, llc, llvm-objcopy; no llvm-as): the previous code
reproduces the CI error and this one produces valid bitcode.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
First of three commits separating artifact FORMAT from link POLICY. Today aiecc decides how to consume a link_with artifact by sniffing its file suffix: `.ll`/`.bc` means "llvm-link this into the core module", anything else means "object-link it". That conflates two independent concepts, and it permanently forecloses a capability that works -- `ld.lld` accepts bitcode as an LTO link input, but under suffix routing a `.bc` can never reach the linker. It also silently swallows an object file that happens to be named `.ll`. Add `link_merge_files` beside the existing `link_files` so the two policies are distinct lists rather than one list read two ways. CoreOp::verify gains two checks: the deprecated core-level `link_with` conflicts with either canonical list, and one artifact may not appear in both lists (it would be defined twice). The existing `link_with` + `link_files` message is left byte-identical so its test still passes. Inert on its own: nothing populates the new attribute yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Second of three. aie-assign-core-link-files now reads an optional `link_with_mode` on each func.func declaration and splits the artifacts it collects into two lists: no mode -> link_files (object-linked, any suffix) mode = "merge" -> link_merge_files (llvm-linked before codegen) "merge" is the only supported value. A mode without a `link_with`, or an unknown value, is an error. The deprecated core-level `link_with` has nowhere to carry a mode, so it always migrates into the ordinary list. The pass also rejects a device in which one artifact is merged by one core and object-linked by another. This has to be device-scoped, not per-core: aiecc's unified mode compiles every core of a device from a single llvm-linked module, so the second core's ELF would define the artifact's symbols twice. A per-core check does not see it. The error points at the merging core with a note on the object-linking one. Artifacts are collected for every core before any attribute is written, so a rejected device is left untouched rather than half-migrated. The "never called from any core" warning said "its .o file will not be linked"; artifacts are no longer necessarily objects, so it now says "its artifact will not be linked or merged". Still inert in practice: no producer emits link_with_mode yet, so link_merge_files stays empty and link_files is byte-identical to before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Third of three, and the flag day: the consumers and the producer have to change together. The pass now routes merge-mode artifacts into link_merge_files, so an aiecc that still sniffed suffixes would look for them in link_files and find nothing; and emitters that still filtered `.ll`/`.bc` would drop a bitcode the linker is supposed to receive. Emitters (ldscript, BCF): all suffix filtering deleted. Every link_files entry is emitted verbatim, `.bc` included -- lld takes it as an LTO input. link_merge_files is never emitted, so an artifact aiecc merges is never also handed to the linker and each symbol is defined once. aiecc: collectCoreIRLinkFiles reads link_merge_files directly; the deprecated core-level link_with branch disappears, since that attribute can never request merging. collectDeviceIRLinkFiles became fallible and repeats the pass's device-scope conflict check on resolved absolute paths -- aiecc can be handed pre-populated IR that never ran the pass. The --xchesscc rejection is unchanged but better founded: it now refuses an explicit request rather than guessing from a filename. New peano diagnostic: a textual `.ll` in link_files cannot be linked by ld.lld, which reads unrecognized inputs as linker scripts and dies with "malformed number". Name the file and suggest link_with_mode = "merge". Bitcode is deliberately allowed. It mirrors the emitter's precedence, so the deprecated core-level link_with is diagnosed only when it is the attribute actually emitted. IRON: external_func gained link_with_mode with validation; ExternalFunction(inline=True) now emits link_with_mode = "merge". The public keyword stays `inline` -- that is the user's intent, while "merge" is the mechanism. They are deliberately spelled differently: merging only yields inlining because the producer also stamps alwaysinline on the kernel. ExternalFunction no longer silently rewrites an explicit object_file_name to `.ll`. That rewrite existed only to make suffix routing fire; with routing explicit, renaming a caller's artifact has no justification, so a non-IR name with inline=True is now an error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review feedback (PR Xilinx#3399, hunhoffe): use the shared benchmarking helpers rather than a hand-rolled time.perf_counter() loop. Canonical usage is programming_examples/getting_started/00_memcpy/memcpy.py. run_iters is a better fit than the loop it replaces, not just tidier: it reports on-NPU time captured around kernel.wait() separately from end-to-end host latency. The example now quotes the NPU figure, which excludes launch overhead and so makes the per-call delta -- the whole point of the benchmark -- much more legible. Its `warmup` parameter also subsumes the manual warm-up call that primed the JIT cache. e2e is used as a fallback if the runtime reports no npu_time. The stats line stays a custom one-liner rather than print_benchmark(): with two variants, labelled single lines read better than four unlabelled ones. Verification also moves to assert_pass, which prints the PASS! that run.lit / run_strix.lit FileCheck for and sys.exit()s on mismatch. Both variants now run over one shared input, so the final check can assert the example's actual claim -- inlining changes nothing but speed -- instead of checking each variant against its own expected array. The docstring caveat about measuring host latency is narrowed accordingly, and the README's measured table is labelled as end-to-end milliseconds from the previous harness, since the script now prints NPU microseconds and I have no hardware to re-measure on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two authors added link-mode coverage independently and the "routing is metadata, not filename" point ended up asserted in five places. Audit and consolidate, with no loss of coverage. * cpp_link_with_mode_verifier.mlir dropped its link_with-vs-link_files chunk: it was byte-identical to cpp_link_with_both_attrs.mlir, which already covers that pre-existing verifier rule. The file now covers only what link_merge_files introduced. * cpp_link_with_ir_artifacts_fallback.mlir deleted. Its deprecation warning and migration assertions duplicated cpp_link_with_deprecation.mlir, and its emitter assertions duplicated cpp_link_with_emitter_fallback.mlir. Its one unique claim -- that the deprecated core-level link_with ignores the suffix too, since only a func.func declaration can carry link_with_mode -- moves to those two files: a second core naming a .ll in the emitter test, and a MIGRATED-NOT: link_merge_files in the deprecation test. * cpp_link_with_mode_merge.mlir and cpp_link_with_ir_artifacts.mlir both asserted the pass's routing. Split their jobs instead of deleting either: ir_artifacts loses its OPT prefix and RUN line and is now purely the emitters' test, while mode_merge owns pass routing and gains the .bc-object-linked case ir_artifacts was carrying (plus a plain .o, so the deliberately perverse naming does not leave the common case untested). Net: one file, one chunk and five lit invocations removed. Every behaviour still has exactly one owner. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rebasing onto main picks up cd9d998 ("[dyn-seq] Rework IRON Runtime as an eager callback body", Xilinx#3387), which replaced rt = Runtime() with rt.sequence(ty, ty) as (A, B): rt.start(worker) rt.fill(fifo.prod(), A) rt.drain(fifo.cons(), B, wait=True) with a callback body registered at construction, mirroring Worker(core_fn, fn_args); workers now go to Program(..., workers=). The inline test and example were the last two users of the old form and failed with "Runtime.__init__() missing 1 required positional argument: 'seq_fn'". They failed only in CI, which tests the PR merged into main -- the branch itself still had the old Runtime, so this was invisible locally. No behavioural change: same fifos, same handles, same worker. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
inline_kernel.py segfaulted after printing the speedup, inside
np.array_equal in the final correctness check.
Tensor.numpy() does not return host data -- it returns a view over the
XRT buffer's mapped memory:
ptr = self._bo.map()
self._data = np.frombuffer(ptr, dtype=self.dtype).reshape(self._shape)
`y` is local to _bench, so when _bench returns its __del__ runs, drops
the bo, and unmaps that memory. The array handed back to main() is then
dangling, and the crash lands in whatever reads it next -- here numpy,
well away from the cause.
Return a copy instead. Also compute the expected values once up front
rather than reading x at the end; x outlives main() so that was never
unsafe, but it keeps every device read next to the run that produced it.
Verified on aie2p/npu2: runs to completion, and the check it never
previously reached now passes -- the inlined kernel's output is
bit-identical to the object-linked one (1.42x on NPU time).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fifield
force-pushed
the
inline-external-funcs
branch
from
July 29, 2026 03:59
3a06f12 to
e8de405
Compare
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.
External function calls dominate tight IRON loops: control stays in Python while kernels are C++, so a core issues a func.call per tile (issue #3396). Add an opt-in inline path that folds the kernel body directly into the aie.core.
ExternalFunction(inline=True) / compile_cxx_core_function(inline=True): compile the C++ kernel to alwaysinline + linkonce_odr LLVM IR (.ll) via clang -S -emit-llvm instead of a .o. The func.func declaration carries a .ll link_with.
aiecc: link_files entries ending in .ll/.bc are routed to an IR-merge list, llvm-link'd into the core LLVM module before opt/llc, and excluded from object linking (ld-script INPUT() / BCF _include) so each symbol is merged once. The always-inliner folds the alwaysinline body in; linkonce_odr lets globaldce drop the now-dead definition. Per-core and unified paths; Peano only (Chess cannot llvm-link).
inline + symbol_prefix is rejected: an inline kernel is emitted as LLVM IR and cannot be symbol-renamed via llvm-objcopy.
Tests/bench:
Measured Performance (best of 3, iters=200):