fix: strip MLIR %alloc names from VHLS csim output and handle ap_int in nanobind wrapper - #554
Conversation
|
Can you add test cases for this? |
|
Black formatting fixed. Added unit tests in |
9479101 to
fb475cb
Compare
|
Rebased onto current |
|
Hi @chhzh123! Just a gentle ping — it's been about 9 days since the last update. CI is green and all requested tests are in place (12 cases in |
…n nanobind wrapper The VHLS C++ emitter emits raw MLIR SSA value names (e.g. %alloc, %alloc1) into the generated kernel.cpp. These are illegal C++ identifiers and cause g++ to fail with 'expected unqualified-id before % token' when IPModule compiles the csim nanobind wrapper. Fix 1 (vitis.py): Add re.sub(r'%(\w)', r'\1', hls_code) at the start of postprocess_hls_code() to strip the MLIR % sigil from all identifiers. The C++ modulo operator is always followed by a non-word character so this substitution is safe. Fix 2 (ip.py): Update parse_cpp_function() to handle ap_int<N>/ap_uint<N> type names (old regex stopped at '<'). Add resolve_nb_type() to map HLS types to nanobind-compatible stdint types (int8_t, uint16_t, etc.) in the generated nanobind wrapper, with reinterpret_cast back to the HLS type when calling the kernel. Fixes test_three_level_systolic_csim which was the only failing test.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add 3 new tests to test_backend_utils.py: - test_postprocess_realistic_mlir_snippet: the exact MLIR-emitted C++ pattern (int16_t %alloc[4][4]) that originally broke g++, with a C++ modulo on the same code path to verify selective stripping - test_parse_cpp_function_plain_types: regression guard ensuring the broadened _TYPE_TOKEN regex did not break plain-type parsing (int8_t, float, int) - test_generate_nanobind_wrapper_uses_stdint_for_ap_int: integration test that constructs an IPModule from a tempfile and verifies the generated wrapper uses int8_t/uint16_t in the nb::ndarray<> signature while keeping ap_int<N> in the reinterpret_cast body All 12 tests pass (up from 9). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…cs reconciled, HIERARCHY moved to notes, stray symlink removed - Local `main` fast-forwarded ad8da09 -> 36bc03e (origin/main; adds PR cornell-zhang#577 commit 2211c69 and PR cornell-zhang#586 AIE-backend XRT/driver compat). - Merge of origin/main into `next` attempted but conflicts in 6 files (allo/backend/simulator.py, allo/backend/vitis.py, allo/ir/builder.py, allo/ir/visitor.py, mlir/lib/Translation/EmitVivadoHLS.cpp, tests/test_vhls.py); merge aborted and deferred to a dedicated reconciliation session. `next` stays on 3458db1. - Deleted stale local branch fix/hierarchical-dataflow-codegen: PR cornell-zhang#577 merged upstream 2026-05-13 (commit 2211c69); fork copy retained. - Stash triage: dropped two redundant stashes (bare-scalar WIP already on feature/region-bare-scalar-axilite; a stale .claude/scheduled_tasks.lock). Retained one stash (region-scope-stateful cross-branch edits) whose untracked payload is unlanded innovation absent from `next` (729 lines: tests/dataflow/hls_synth_fp16.py, tests/dataflow/test_stream_nb_patterns.py, tests/u280_hw_deploy.py). - Removed stray untracked .antigravitycli directory (gemini config artifact); its external symlink target under /home/sk3463/.gemini was left untouched. - Moved ALLO_HIERARCHY_DESIGN.md -> notes/HIERARCHY_DESIGN.md (now tracked). - Documented feature/mesh-accelerator (06ce561) as an intentionally-parked archival line (22 unique exploratory commits: Catapult-v1 end-to-end, mesh-v1, decoupled-mesh perf-eval framework); feature/mesh-accelerator-v2 (b73b555) is the maintained successor already in `next`. - Reconciled STATE.md and BRANCHES.md: cornell-zhang#577 moved to merged/done, cornell-zhang#554 kept OPEN (CI green, rebase-clean, re-ping pending), closed cornell-zhang#578/cornell-zhang#563/cornell-zhang#562 recorded for history, fork-first posture recorded, feature/region-bare-scalar-axilite noted as reverted-from-next (revert a7ae144) reference-only, HIERARCHY doc references updated to notes/HIERARCHY_DESIGN.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HiarofTe41DMq8rkXyUyPG
fb475cb to
ad95efd
Compare
|
Hi @Fangtangtang, I have rebased this branch onto the current main and CI is green. Would you mind taking a look when you have a moment? Thanks in advance for the review. |
I noticed the unit test CI did not trigger due to a runner configuration issue. I have just fixed it (in #594). Could you please merge the latest main into your branch and rerun the CI? Thanks! |
Fangtangtang
left a comment
Fangtangtang
left a comment
There was a problem hiding this comment.
Thanks for your work on this! Sorry for the delayed review. I left a few comments and suggestions below.
Also, could you also add some documentation (update docs/source/dive/ip.rst) for the supported data types in the IP module?
… (per reviewer request on cornell-zhang#554)
…_use_def Names derived from textual SSA identifiers (e.g. "%alloc", "%0") were persisted verbatim into the "name" attribute in analyze_use_def and then leaked into the C++ emitted by all HLS backends, producing illegal identifiers such as `int16_t %alloc[4][4]`. The "%" sigil is stripped once at the source so both the buffer-name key and the stored attribute use the sigil-free name consistently. Fixing at the source makes the string postprocess in allo/backend/vitis.py (a re.sub that stripped "%" from already-emitted HLS code) unnecessary, so it and the now-unused `import re` are removed. The vhls backend, which never ran that postprocess, was still emitting stray sigils; this fix covers it as well. Surfaced via csim of tests/test_systolic_array.py (test_three_level_systolic_csim), where buffer_at + partition give an alloc a textual-SSA-derived name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ression test Relocate tests/test_backend_utils.py to tests/utils/test_backend_utils.py. The postprocess_hls_code unit tests that asserted "%"-stripping are removed: that behavior moved to the source fix in analyze_use_def, and postprocess_hls_code no longer touches "%". They are replaced by test_no_ssa_sigil_in_emitted_hls_code, which drives the analyze_use_def naming path (buffer_at + partition) up to vhls emission and asserts no SSA sigil survives - fast and Vitis-free. The modulo-preservation test is kept as a guard on postprocess_hls_code's remaining behavior, and the resolve_nb_type / parse_cpp_function / generate_nanobind_wrapper tests (which exercise ip.py) are unchanged. Add a functional IP regression test for ap_int element types: tests/ip_integration/vadd_ap_int.cpp plus test_ap_int in tests/ip_integration/test_external.py, gated on Vitis HLS availability with link_hls=True, passing np.int8 A/B and np.int16 C and checking numeric correctness. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add a "Supported data types" subsection to docs/source/dive/ip.rst listing the element types allowed in a C++ IP signature: standard C arithmetic types (int, float, double, fixed-width stdint types) plus ap_int<N> and ap_uint<N>. Explain that ap_int/ap_uint map to the matching fixed-width integer type at the Python boundary (so callers pass NumPy arrays of the matching dtype) and that using them requires link_hls=True. Include a vadd_ap_int example consistent with the new tests/ip_integration fixture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the review! All three points addressed. Root cause fixed at the source, postprocessing removed. You were right that this is a renaming bug, but it turned out to live one step before the emitter: Tests. Moved the function-level unit tests to Docs. Added a "Supported data types" section to |
Fangtangtang
left a comment
Fangtangtang
left a comment
There was a problem hiding this comment.
@chhzh123 Could you take a look at the ap_int/ap_uint support part (in allo/backend/ip.py and docs/source/dive/ip.rst) of this PR? I'm not very familiar with the HLS backend.
| Supported data types | ||
| ==================== | ||
|
|
||
| The element types allowed in the C++ IP signature are the standard C arithmetic types (``int``, ``float``, ``double``, and the fixed-width ``stdint`` types such as ``int8_t``/``uint16_t``), as well as the arbitrary-precision HLS integer types ``ap_int<N>`` and ``ap_uint<N>``. Because ``ap_int``/``ap_uint`` are provided by the Vitis HLS headers, an IP module that uses them must be created with ``link_hls=True``. |
There was a problem hiding this comment.
provided by the Vitis HLS headers
shall we use "Vitis HLS libraries" instead to keep the terminology consistent with the documentation?
Also @chhzh123, I wonder if the naming link_hls should be more specific. Would link_vitis_hls be more accurate?
There was a problem hiding this comment.
I think this should support different HLS backends, but currently, only Vitis HLS is supported, so for other backends, it should error out, and this could be another PR.
…feature status Consolidates notes/, fork issues #4-#12, and upstream PRs cornell-zhang#554/cornell-zhang#577/cornell-zhang#579/cornell-zhang#593 into a single feature-level map with status lanes and next actions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
chhzh123
left a comment
chhzh123
left a comment
There was a problem hiding this comment.
The ap_int part looks good, just a minor issue about the test case
| Supported data types | ||
| ==================== | ||
|
|
||
| The element types allowed in the C++ IP signature are the standard C arithmetic types (``int``, ``float``, ``double``, and the fixed-width ``stdint`` types such as ``int8_t``/``uint16_t``), as well as the arbitrary-precision HLS integer types ``ap_int<N>`` and ``ap_uint<N>``. Because ``ap_int``/``ap_uint`` are provided by the Vitis HLS headers, an IP module that uses them must be created with ``link_hls=True``. |
There was a problem hiding this comment.
I think this should support different HLS backends, but currently, only Vitis HLS is supported, so for other backends, it should error out, and this could be another PR.
| buf_A = s.buffer_at(s.A, "i") | ||
| buf_B = s.buffer_at(s.B, "j") | ||
| pe = s.unfold("PE", [0, 1]) | ||
| s.partition(s.C, dim=0) | ||
| s.partition(s.A, dim=1) | ||
| s.partition(s.B, dim=2) | ||
| s.to(buf_A, pe, axis=1, depth=M + 1) | ||
| s.to(buf_B, pe, axis=0, depth=N + 1) |
There was a problem hiding this comment.
Try to simplify this test case? Do we need so many primitives to reproduce the issue?
Problem
When running
test_three_level_systolic_csim, the Vitis HLS C++ emitter generateskernel.cppcontaining raw MLIR SSA names like%alloc[1]— invalid C++ identifiers. This causesg++to fail with:A secondary issue: [parse_cpp_function()](cci:1://file:///home/sk3463/main/projects/allo/allo/backend/ip.py:30:0-112:17) in
ip.pyused\w+to match C++ types, which stops at<, soap_int<8>was misread (only8was captured as the type name), producing invalid generated nanobind wrapper code.Fix
vitis.py: Addre.sub(r'%(\w)', r'\1', hls_code)at the start of [postprocess_hls_code()](cci:1://file:///home/sk3463/main/projects/allo/allo/backend/vitis.py:377:0-429:18). The MLIR%sigil always precedes a word character; the C++ modulo operator does not. Safe substitution.ip.py: Broaden type-matching regex to handleap_int<N>/ap_uint<N>types. Add [resolve_nb_type()](cci:1://file:///home/sk3463/main/projects/allo/allo/backend/ip.py:16:0-27:19) to map HLS types toint{N}_tfor the nanobind interface, withreinterpret_castback to the HLS type when calling the kernel.Testing
tests/test_systolic_array.py::test_three_level_systolic_csimnow passes. This test only runs when Vitis HLS is available, which is why it was not caught by upstream CI.