GAP9: -O3 hot kernels + L2 tiling/DMA tables#197
Closed
runwangdl wants to merge 14 commits into
Closed
Conversation
deeploytest.c classified memory by `ptr >= 0x10000000` (inputs) / `< 0x10000000` (outputs). HyperRAM/L3 addresses (cl_ram_malloc) are also >= 0x10000000 but are NOT CPU-addressable, so for `--defaultMemLevel L3` tests on real silicon main did a raw memcpy / CPU-deref of an L3 pointer -> 'Invalid fetch' fault in main (e.g. MatMul L3 on board: fault at the cl_ram_malloc'd input address). GVSoC models HyperRAM as flat RAM so it passed there, masking the bug. Add IS_L1/IS_L2 on-chip-window macros and use them: - Inputs: only memcpy on-chip (IS_L2) inputs with a non-NULL testInputVector; L3 inputs are loaded from the readfs hex in InitNetwork (testInputVector is NULL) and already live in HyperRAM, so skip them. - Outputs: ram_read L3 outputs into an L2 scratch before the compare (and free it); on-chip outputs compared in place. Paired malloc/free kept in sync. Verified MatMul --defaultMemLevel L3 on GVSoC: 0/256 (unchanged). On-chip (L2) tests behave identically; only L3 paths change.
The L3<->L2 tiling used one DMA backend for both single- and double-buffering.
Async DMA only helps double-buffering (it overlaps the next-tile prefetch with
compute); single-buffering waits on each tile before computing, so async gives
SB no benefit but all the risk — strided 2D L3 transfers (pi_cl_ram_copy_2d) can
corrupt under deferred waits.
- PULPL3Tiling: add optional `dbDma` (defaults to `dma`) so SB and DB can use
different backends. Backward compatible.
- GAP9 bindings: SB keeps the blocking gap9L3DmaHack; DB uses async GAP9L3Dma for
real L3<->L2 prefetch overlap.
- GAP9L3Dma: reset future `.size`=0 after copy-wait (so a completed future isn't
waited twice) and cast `${ext}` to uint32_t in the 2D transfer.
Verified MatMul --defaultMemLevel L3 on GVSoC: 0/256.
Contributor
📝 WalkthroughWalkthrough
ChangesAsync DB DMA engine split in PULPL3Tiling and GAP9 wiring
IS_L1/IS_L2 macro-based address detection in GAP9 test harness
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DeeployTest/Platforms/GAP9/src/deeploytest.c (1)
178-184:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing NULL check on
pi_l2_mallocmay cause crash.If L2 memory is exhausted,
pi_l2_mallocreturns NULL, and the subsequentram_readand comparison will dereference NULL, causing a fault. Adding a check with an error message provides clearer diagnostics than an opaque crash.🛡️ Proposed fix to handle allocation failure
if (!IS_L2(DeeployNetwork_outputs[buf])) { compbuf = pi_l2_malloc(DeeployNetwork_outputs_bytes[buf]); + if (compbuf == NULL) { + printf("Error: failed to allocate L2 scratch for output %u (%u bytes)\r\n", + buf, DeeployNetwork_outputs_bytes[buf]); + return -1; + } ram_read(compbuf, DeeployNetwork_outputs[buf], DeeployNetwork_outputs_bytes[buf]); } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DeeployTest/Platforms/GAP9/src/deeploytest.c` around lines 178 - 184, The pi_l2_malloc call may return NULL when L2 memory is exhausted, but the code does not check for this failure before passing compbuf to ram_read, which will cause a NULL pointer dereference. Add a NULL check immediately after the pi_l2_malloc call (when IS_L2(DeeployNetwork_outputs[buf]) is false) and handle the error case with an appropriate error message or failure path to prevent the crash.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@DeeployTest/Platforms/GAP9/src/deeploytest.c`:
- Around line 178-184: The pi_l2_malloc call may return NULL when L2 memory is
exhausted, but the code does not check for this failure before passing compbuf
to ram_read, which will cause a NULL pointer dereference. Add a NULL check
immediately after the pi_l2_malloc call (when IS_L2(DeeployNetwork_outputs[buf])
is false) and handle the error case with an appropriate error message or failure
path to prevent the crash.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31b0183f-da2c-45e2-99d4-f43e1f5d2bbe
📒 Files selected for processing (4)
Deeploy/Targets/GAP9/Bindings.pyDeeploy/Targets/GAP9/DMA/L3Dma.pyDeeploy/Targets/PULPOpen/CodeTransformationPasses/PULPL3Tiling.pyDeeployTest/Platforms/GAP9/src/deeploytest.c
…mple
- TargetLibraries/GAP9: compile the hot forward kernels (Convolution_fp32,
DWConvolution_fp32, Gemm) at -O3, appended last so it wins over the SDK's
default -Os. These dominate GAP9 inference cycles.
- deeploytest.c / CMake / sdk config: make the GAP9 example show the three
L1-memory knobs that let conv-heavy nets fit, with explanatory comments:
A. slave (PE) stacks -> L2: hand the cluster task a static L2 buffer
(SET_SLAVE_STACK) so the SDK skips its L1 slave-stack alloc (~30 KB L1).
B. shrink the SDK's L1 slave stacks via CONFIG_CL_SLAVE_CORE_STACK_SIZE
(sdk_gvsoc.config) -- alternative to A.
C. size the cluster-controller stack via conf.cc_stack_size, overridable
from the build with -DCC_STACK_SIZE=<bytes> (new CMake option).
Verified MatMul --defaultMemLevel L3 -DCC_STACK_SIZE=8192 on GVSoC: 0/256.
…stack) The tiling argument structs were stack-locals in the dispatching function. The cluster fork runtime writes its descriptor near the top of the CC/master stack; a stack-local arg struct placed there can be clobbered before the forked cores read it (a GAP9 cluster-fork crash, e.g. MobileNetV1). Declare the struct `static` and assign separately so it lives in static storage, stable across the forked call. Generic codegen (ArgumentStructGeneration); benign on other targets. Verified MatMul --defaultMemLevel L3 on GVSoC: 0/256.
GAP9 mchan allocates a fresh channel on every descriptor enqueue. The previous DirectionWaitingStrategy shares one future (one mchan_transfer_get_id) across all same-direction tensors of a tile, so a tile with >1 input emits one get_id but multiple pushes -> the extra transfers run on channels that are never waited or freed -> mchan_transfer_wait() hangs (e.g. the optimizer weight+grad stall). Switch to PerTensorWaitingStrategy so each tensor gets its own get_id : push : wait : free, matching the mchan contract. Verified MatMul --defaultMemLevel L3 on GVSoC: 0/256.
Explain each GAP9 backend change in this branch — L3-aware board harness, SB/DB L3 DMA split, -O3 forward kernels, the three L1-memory knobs (cc_stack / slave-stack size / slave-stack->L2), static cluster fork/closure args, and the per-tensor mchan DMA waiting strategy — with problem, fix, file, and takeaway, plus a short GAP9 memory-model primer.
Add gap9_memcheck.py and run it from run_complete_test after the build, before the simulation, on GAP9. It models every consumer of L1/L2 the tiler doesn't (CC master stack, PE slave stacks, ELF sections, tile arena, promoted pool) and scans InitNetwork for the pi_l2_malloc-after-cl_ram_malloc alloc-order race, so over-subscription fails fast with the exact knob instead of a multi-minute GVSoC hang. GAP9-only; bypass with DEPLOY_SKIP_MEMCHECK=1. Verified MatMul L3: gate runs (PASS) and test is 0/256.
… cost Add section 8: the per-tile tiling metadata AND DMA descriptor (_cmd) tables are static PI_L2, so they cost the L1 arena nothing. Measured L1 saved per training net: AE 283B, DSCNN 497B, ResNet8 2.5KB, MNv1 7.0KB, CCT 11.6KB (DMA tables are 25-32% of that). Access cost is negligible: relocating AE's tables L2->L1 is only 0.6% faster (3,111,938 -> 3,093,012 cyc), so L2 is the right home. Load-bearing for CCT (L1 full). Correct Knob A (slave stacks -> L2): it is NOT free — measured +22..63% train cycles (hot stack accesses hit L2). Prefer shrinking SLAVESTACKSIZE (<256B real need) and keeping stacks in L1; use L2 stacks only when the arena needs every byte. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Keep only the two highest-value, low-risk pieces: - -O3 on the hot conv/dwconv/gemm kernels (TargetLibraries/GAP9/CMakeLists.txt). - docs: rewrite to a focused note on the -O3 win and on where the tiling + DMA descriptor tables live (L2) and how much L1 that keeps free (per-model table). Reset the rest to base: the L3/mchan DMA changes (Bindings, L3Dma, MchanDma, PULPL3Tiling), the L3-aware I/O harness change (deeploytest.c), the build-time memory gate (gap9_memcheck.py + execution.py), and the DeeployTest cmake/sdk config knobs. Those can land separately.
The GAP9 global-init templates emitted compile-time-initialized arrays as
'static PI_L2 ... = {...}'. The per-tile tiling-codegen and DMA descriptor (_cmd)
tables go through these — they are read-only lookup tables (the cluster controller
only reads them to drive the tiling loop and program DMAs). Emit them
'static const PI_L2 ...': correct (never written), immutable read-only data.
Verified: all 228 tables of a model become const, builds with 0 const-discard,
runs identically. Doc updated with the change and the L2/L1 table-placement note.
runwangdl
changed the title
Render the GAP9 global-init templates and assert the compile-time constant arrays (which the hoisted tiling-codegen + DMA descriptor tables route through) come out 'static const PI_L1/L2'. Guards against the const being dropped. Verified it passes with the const templates and fails without.
Fixes the failing pre-commit (Autoformat Python Files / yapf) on the test added in the previous commit; formatting only, no behavior change.
runwangdl
changed the title
The hoisted tiling-codegen + DMA descriptor tables are forced to L2 (_memoryLevel == 'L2'), so they render through the L2 global-init templates; no tiling/DMA table is ever placed in L1. Revert the const on the L1 init templates (unnecessary for these tables) and keep it on the L2 ones. Verified: all 228 tables of a model still come out const. Test narrowed to the L2 templates.
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
1 participant
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.
GAP9 backend, pared to two focused, low-risk changes. The L3/mchan DMA, L3-aware I/O, and build-time memcheck changes were reset to land separately.
1.
-O3on the hot kernelsTargetLibraries/GAP9/CMakeLists.txt— compile the conv / depthwise-conv / Gemm (+ backward) kernels at-O3, appended last so it wins over the SDK's global-Os. This turns on the RISC-V (XpulpV2) hardware loops on the kernels' tight inner loops.Kernel benchmark —
Tests/Kernels/FP32/Conv/Regular_2D_Bias, GAP9 GVSoC:lp.setup(HW loops).text-Os(SDK default)-O3(this PR)−12.3% cycles on this conv (the compute-bound matmul / conv-grad kernels gain more), hardware loops 0 → 18, at +50%
.texton the few hot files.2. Tiling + DMA descriptor tables →
const, and they live in L2Deeploy/Targets/GAP9/Templates/AllocateTemplate.py_cmd) tables asstatic const PI_L2 … = {…}— they are read-only lookup tables (the cluster controller only reads them to drive the tiling loop / program DMAs). Verified: 228 tables of a model becomeconst, builds with 0 const-discard, runs identically. Guarded bytest_gap9_constant_tables_are_const(DeeployTest/test_deeploy_internal.py).PI_L2, so they live in L2 and cost the L1 tile arena nothing — keeping up to ~11.6 KB of L1 free (CCT; DMA tables are 25–32% of that). Per-model table indocs/gap9_backend_fixes.md.Placing them in L2 is latency-neutral — measured by relocating all tiling+DMA tables
PI_L2→PI_CL_L1on a GEMM workload (Autoencoder) and re-running, GAP9 GVSoC:i.e. moving the tables into fast L1 buys back <1% — so keeping them in L2 (which frees that L1 for the tile arena) costs essentially nothing.