Add SoCDAML Part III: hands-on lab for adding a new int8 operator - #194
Add SoCDAML Part III: hands-on lab for adding a new int8 operator#194viv-eth wants to merge 11 commits into
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds Part III iLeakyReLU student skeletons and a complete reference solution with deterministic artifacts, scalar/SIMD kernels, parsing, templating, tiling, deployment wiring, runner flag forwarding, and expanded tutorial documentation. ChangesiLeakyReLU Part III
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant generate.py
participant deploy.sh
participant Deeploy
User->>generate.py: generate ONNX and golden tensors
generate.py-->>User: network.onnx, inputs.npz, outputs.npz
User->>deploy.sh: select scalar or SIMD mode
deploy.sh->>Deeploy: copy artifacts and patch integration files
User->>Deeploy: run iLeakyReLU test
Deeploy-->>User: generated and executed kernel results
Possibly related PRs
Suggested labels: 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}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 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.
Inline comments:
In `@docs/tutorials/introduction.md`:
- Line 58: Update the introductory description to call the instructions a
“six-step sequence” instead of a “four-step sequence,” leaving the surrounding
Docker and Singularity translation text unchanged.
- Line 115: Update the fenced code blocks in docs/tutorials/introduction.md,
including the blocks near lines 115, 371, 385, 485, 523, and 595, to specify
language identifiers: use text for error-output content and bash for
shell-command examples. Preserve the existing block contents.
- Line 261: Update the profiling description near the --profileTiling flag to
say the transformation inserts print statements displaying runtimes, and revise
the example introduction to say profiling will print the following.
- Line 347: In the tutorial introduction paragraph, update the sentence near
“add your own operator from scratch” to replace the duplicated article in “the
an int8 LeakyReLU” with “an int8 LeakyReLU.”
- Around line 93-97: Update the “Build the writable Singularity sandbox”
instructions to use the course-supported unprivileged build path, such as the
required --fakeroot invocation, and document any necessary subuid/subgid setup
or supported remote-build alternative. Keep the Deeploy image and writable
sandbox behavior unchanged.
- Around line 68-70: Update the tutorial’s Singularity shell setup so CCACHE_DIR
and PIP_CACHE_DIR remain available when --cleanenv is used; pass both variables
through the container environment or export them immediately after entering the
shell, preserving their $SCRATCH-based values.
In `@Tutorials/PartIII_skeletons/iLeakyReLU/generate.py`:
- Around line 55-56: Update the NPZ generation in the tutorial script to save
the arrays under the Deeploy-compatible keys input and output, converting both x
and y to int64 before writing. Preserve the existing input/output data
generation while replacing data_in/data_out and int8 storage.
In `@Tutorials/PartIII_skeletons/iLeakyReLU/iLeakyReLU_simd.c`:
- Around line 36-39: Update the per-core partitioning in the iLeakyReLU SIMD
worker so rounding does not drop elements or produce zero-sized chunks.
Partition complete 4-element vectors by vector index across cores, then ensure
the size % 4 scalar tail is processed exactly once, preserving coverage of every
element even when size is smaller than or not evenly divisible by nC.
In `@Tutorials/PartIII_skeletons/iLeakyReLU/README.md`:
- Line 12: Update the iLeakyReLU_simd.c entry in the README table to state that
packed shift, max, and final store remain TODOs for the student, while retaining
the existing SIMD chunking context and Step 6b guidance.
In `@Tutorials/PartIII_solution/iLeakyReLU/deploy.sh`:
- Around line 35-43: Update the undo branch in deploy.sh to preserve
pre-existing contents of TESTS_DIR: remove only the copied iLeakyReLU test
artifacts, and remove TESTS_DIR only when it is empty. Keep the cleanup of the
three iLeakyReLU source, header, and template/constraint files unchanged.
In `@Tutorials/PartIII_solution/iLeakyReLU/generate.py`:
- Line 77: Update the integer fixture generation in the visible RNG call to
include the positive int8 boundary by using an exclusive upper bound of 128 or
enabling the inclusive endpoint. Regenerate the corresponding NPZ artifacts
after changing the input distribution.
In `@Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLU_simd.c`:
- Around line 36-57: Fix partitioning in
Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLU_simd.c lines 36-57 by
distributing complete v4s blocks across cores without rounding each core’s work
to zero or discarding remainders, then have one core process the global scalar
remainder. Update
Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLUTileConstraint.py lines 39-51 to
remove the incorrect tail-free guarantee or enforce only constraints still
required by the corrected SIMD partition; use the existing SIMD loop and scalar
tail symbols as integration points.
In `@Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLUParser.py`:
- Around line 22-35: Constrain iLeakyReLU parsing to the SIMD contract in both
iLeakyReLUParser.parseNode and the injected parser in deploy.sh: accept only mul
== 1 and a supported non-negative shift, rejecting unsupported parameter
combinations before populating operatorRepresentation. Keep both parser
implementations consistent unless the SIMD implementation is updated to apply
multiplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: baff3e72-3778-4433-a52c-cda43b737a42
⛔ Files ignored due to path filters (5)
Tutorials/Images/DeeploySystem.pngis excluded by!**/*.pngTutorials/Images/EthLogoNeg.pngis excluded by!**/*.pngTutorials/Images/Siracusa.pngis excluded by!**/*.pngTutorials/Images/Victor_Jung_EDGEAIForumDeeploy_S5.pngis excluded by!**/*.pngdocs/img/DeeployLogoGreen.svgis excluded by!**/*.svg
📒 Files selected for processing (25)
.gitignoreCHANGELOG.mdDeeployTest/testUtils/deeployRunner.pyTutorials/PartIII_skeletons/iLeakyReLU/README.mdTutorials/PartIII_skeletons/iLeakyReLU/generate.pyTutorials/PartIII_skeletons/iLeakyReLU/iLeakyReLU.cTutorials/PartIII_skeletons/iLeakyReLU/iLeakyReLU.hTutorials/PartIII_skeletons/iLeakyReLU/iLeakyReLUParser.pyTutorials/PartIII_skeletons/iLeakyReLU/iLeakyReLUTemplate.pyTutorials/PartIII_skeletons/iLeakyReLU/iLeakyReLUTileConstraint.pyTutorials/PartIII_skeletons/iLeakyReLU/iLeakyReLU_simd.cTutorials/PartIII_solution/iLeakyReLU/README.mdTutorials/PartIII_solution/iLeakyReLU/deploy.shTutorials/PartIII_solution/iLeakyReLU/generate.pyTutorials/PartIII_solution/iLeakyReLU/iLeakyReLU.cTutorials/PartIII_solution/iLeakyReLU/iLeakyReLU.hTutorials/PartIII_solution/iLeakyReLU/iLeakyReLUParser.pyTutorials/PartIII_solution/iLeakyReLU/iLeakyReLUTemplate.pyTutorials/PartIII_solution/iLeakyReLU/iLeakyReLUTileConstraint.pyTutorials/PartIII_solution/iLeakyReLU/iLeakyReLU_simd.cTutorials/PartIII_solution/iLeakyReLU/inputs.npzTutorials/PartIII_solution/iLeakyReLU/network.onnxTutorials/PartIII_solution/iLeakyReLU/outputs.npzdocs/tutorials/debugging.rstdocs/tutorials/introduction.md
| ```bash | ||
| docker run -it --name deeploy_main -v $(pwd):/app/Deeploy ghcr.io/pulp-platform/deeploy:main | ||
| ``` | ||
| is the four-step sequence below. The key part of the translation: Docker's `-v $(pwd):/app/Deeploy` (bind-mount the host clone) becomes Singularity's `--bind "$SCRATCH/Deeploy:/app/Deeploy"`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the installation step count.
Line 58 says “four-step sequence,” but the instructions contain six numbered steps. Change it to “six-step sequence.”
[details]
🤖 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 `@docs/tutorials/introduction.md` at line 58, Update the introductory
description to call the instructions a “six-step sequence” instead of a
“four-step sequence,” leaving the surrounding Docker and Singularity translation
text unchanged.
There was a problem hiding this comment.
@Victor-Jung, thanks for confirming. The introductory wording should be updated to say “six-step sequence” so it matches the six numbered installation steps.
| export SINGULARITY_CACHEDIR="$SCRATCH/.singularity_cache" | ||
| export CCACHE_DIR="$SCRATCH/.ccache" | ||
| export PIP_CACHE_DIR="$SCRATCH/.pip_cache" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## candidate file section"
if [ -f docs/tutorials/introduction.md ]; then
nl -ba docs/tutorials/introduction.md | sed -n '55,120p'
else
echo "docs/tutorials/introduction.md not found"
fd -a 'introduction\.md$' .
fi
echo
echo "## search for cleanenv/cache env context"
rg -n --fixed-strings "--cleanenv|SINGULARITY_CACHEDIR|APPTAINER_CACHEDIR|CCACHE_DIR|PIP_CACHE_DIR|APPTAINERENV|SINGULARITYENV" docs || true
echo
echo "## relevant tutorial command snippets"
rg -n --fixed-strings "singularity exec|apptainer exec|bash --login|APPTAINERENV|SINGULARITYENV|--cleanenv|SINGULARITY_CACHEDIR|CCACHE_DIR|PIP_CACHE_DIR" docs || trueRepository: pulp-platform/Deeploy
Length of output: 225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## candidate file section"
if [ -f docs/tutorials/introduction.md ]; then
awk '{printf "%6d\t%s\n", NR, $0}' docs/tutorials/introduction.md | sed -n '55,120p'
else
echo "docs/tutorials/introduction.md not found"
find . -type f -name 'introduction.md' | sed -n '1,20p'
fi
echo
echo "## search for cleanenv/cache env context"
grep -RInE -- --cleanenv|SINGULARITY_CACHEDIR|APPTAINER_CACHEDIR|CCACHE_DIR|PIP_CACHE_DIR|APPTAINERENV|SINGULARITYENV docs || true
echo
echo "## relevant tutorial command snippets"
grep -RInE -- 'singularity exec|apptainer exec|bash --login|--(env|cleanenv)|APPTAINERENV|SINGULARITYENV|SINGULARITY_CACHEDIR|CCACHE_DIR|PIP_CACHE_DIR' docs || trueRepository: pulp-platform/Deeploy
Length of output: 5156
🌐 Web query:
Singularity/Apptainer --cleanenv environment variables APPTAINERENV_ SINGULARITYENV documentation
💡 Result:
In Apptainer, the --cleanenv (or -e) option and environment variable prefixes are used to manage the runtime environment of a container [1][2]. --cleanenv / -e The --cleanenv option (or its alias -e) prevents host environment variables from being automatically passed into the container [1][3]. When used, the container starts with a minimal set of environment variables necessary for core operation [1][3]. This is particularly useful when host systems set numerous variables (e.g., via environment modules or lmod) that might interfere with software inside the container [1][3]. You can also control this behavior via the environment variable APPTAINER_CLEANENV [4]. APPTAINERENV_ and SINGULARITYENV_ Prefixes These prefixes allow you to inject environment variables into a container at runtime [5][4]. - APPTAINERENV_PREFIX: Setting an environment variable on the host with the prefix APPTAINERENV_ (e.g., APPTAINERENV_FOO=BAR) will create an environment variable named FOO with the value BAR inside the container [5][4]. - Compatibility with SINGULARITYENV_: Apptainer maintains backward compatibility with the legacy SINGULARITYENV_ prefix [5][6]. If a corresponding APPTAINERENV_ variable is not set, Apptainer will respect the SINGULARITYENV_ prefix [5][6]. - Precedence: If both the APPTAINERENV_ and SINGULARITYENV_ versions of a variable are defined, the value set by the APPTAINERENV_ variable takes priority [5][6]. Apptainer will typically issue a warning if both are present to alert you to the potential conflict [5][6]. The use of the SINGULARITYENV_ prefix is considered deprecated, and users are encouraged to transition to the APPTAINERENV_ prefix to ensure long-term compatibility [5][6].
Citations:
- 1: https://apptainer.org/docs/user/latest/environment_and_metadata.html
- 2: https://apptainer.org/docs/user/main/cli/apptainer_exec.html
- 3: https://apptainer.org/docs/user/main/environment_and_metadata.html
- 4: https://apptainer.org/user-docs/master/appendix.html
- 5: https://apptainer.org/docs/user/latest/singularity_compatibility.html
- 6: https://apptainer.org/docs/user/main/singularity_compatibility.html
Preserve the cache variables through --cleanenv.
Steps 68-70 export CCACHE_DIR and PIP_CACHE_DIR, but step 109 runs singularity shell ... --cleanenv, which does not automatically carry those host variables into the shell. With the shell’s own defaults, ccache/pip can fall back to $HOME and consume the quoted home filesystem. Pass them with --env/APPTAINERENV_/SINGULARITYENV_, or export them after entering the shell.
🤖 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 `@docs/tutorials/introduction.md` around lines 68 - 70, Update the tutorial’s
Singularity shell setup so CCACHE_DIR and PIP_CACHE_DIR remain available when
--cleanenv is used; pass both variables through the container environment or
export them immediately after entering the shell, preserving their
$SCRATCH-based values.
There was a problem hiding this comment.
🧩 Analysis chain
🤖 web_search executed:
Apptainer official documentation --cleanenv host environment variables --env SINGULARITYENV CCACHE_DIR PIP_CACHE_DIR
Length of output: 15995
@Victor-Jung, checked: the finding is valid. Both Apptainer and SingularityCE document that --cleanenv drops regular host environment variables; CCACHE_DIR and PIP_CACHE_DIR therefore are not carried into the shell as currently written. SINGULARITY_CACHEDIR is only needed for the host-side singularity build, so that one is fine.
One extra detail for the fix: forwarding $SCRATCH/.ccache and $SCRATCH/.pip_cache with --env only works if those paths are also visible inside the container. Since the tutorial says the scratch CWD is not available in the container, the revised command should explicitly bind the cache directory (or otherwise use a container-visible scratch path), then set the two variables via --env.
So this should remain open, with the documentation updated to cover both the bind mount and environment forwarding.
| ### Profiling the Execution | ||
|
|
||
| To measure the effect of some optimizations in more detail, you can use the `--profileTiling=L2` flag. This flag will enable a code transformation that will insert print displaying the runtime of several critical code sections. For instance, profiling an *Integer Layer Normalization* layer from L2 with two tiles will return the print the following: | ||
| To measure the effect of some optimizations in more detail, you can use the `--profileTiling` flag. This flag will enable a code transformation that will insert print displaying the runtime of several critical code sections. For instance, profiling an *Integer Layer Normalization* layer from L2 with two tiles will return the print the following: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the profiling sentence grammar.
The text currently says “insert print displaying” and “will return the print the following.” Use “insert print statements displaying” and “will print the following.”
🤖 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 `@docs/tutorials/introduction.md` at line 261, Update the profiling description
near the --profileTiling flag to say the transformation inserts print statements
displaying runtimes, and revise the example introduction to say profiling will
print the following.
There was a problem hiding this comment.
@Victor-Jung, agreed — this is a documentation nitpick.
| | `generate.py` | Complete ONNX + golden-value generator | Run it (Step 1) | | ||
| | `iLeakyReLU.h` | Complete kernel header | Copy to `TargetLibraries/PULPOpen/inc/kernel/` (Step 3) | | ||
| | `iLeakyReLU.c` | Multi-core chunking provided; inner loop TODO | Fill the TODO, copy to `TargetLibraries/PULPOpen/src/` (Step 3) | | ||
| | `iLeakyReLU_simd.c` | SIMD chunking + load/max/store provided; one TODO line | Fill in Step 6b after the scalar works | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the SIMD description with the skeleton.
Line 12 says load/max/store are provided, but iLeakyReLU_simd.c leaves the packed shift, max, and final store for the student. Update this row so the lab instructions match the actual file.
🤖 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 `@Tutorials/PartIII_skeletons/iLeakyReLU/README.md` at line 12, Update the
iLeakyReLU_simd.c entry in the README table to state that packed shift, max, and
final store remain TODOs for the student, while retaining the existing SIMD
chunking context and Step 6b guidance.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Tutorials/PartIII_skeletons/iLeakyReLU/README.md ---'
cat -n Tutorials/PartIII_skeletons/iLeakyReLU/README.md
printf '%s\n' '--- TODOs and SIMD implementation context ---'
rg -n -C 5 'TODO\(student\)|vOut|vMax|shift|store|load' \
Tutorials/PartIII_skeletons/iLeakyReLU/iLeakyReLU_simd.cLength of output: 4223
@Victor-Jung, checked: this is still valid.
The skeleton provides SIMD chunking and the vector load, but the packed shift, packed signed max, and final output store remain TODO(student) work—the current vOut[i] = x is only a placeholder.
A matching Line 12 description would be:
| `iLeakyReLU_simd.c` | SIMD chunking + vector load provided; packed shift/max/store TODO | Fill in Step 6b after the scalar works || undo) | ||
| echo "Undoing iLeakyReLU additions (file copies only)..." | ||
| rm -rf "$TESTS_DIR" | ||
| rm -f "$KERNEL_SRC_DIR/iLeakyReLU.c" | ||
| rm -f "$KERNEL_INC_DIR/iLeakyReLU.h" | ||
| rm -f "$TEMPLATES_DIR/iLeakyReLUTemplate.py" | ||
| rm -f "$TILECONSTR_DIR/iLeakyReLUTileConstraint.py" | ||
| echo "Note: hand-patches in Parsers.py / Bindings.py / Platform.py / Tiler.py / DeeployPULPMath.h were NOT removed." | ||
| echo "If you need a fully clean tree, use: git checkout -- Deeploy/Targets TargetLibraries/PULPOpen/inc/DeeployPULPMath.h" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not delete the entire test directory.
Line 37 removes any pre-existing tests or local artifacts under this shared path. Delete only the three copied artifacts and optionally remove the directory if empty.
Proposed fix
- rm -rf "$TESTS_DIR"
+ rm -f "$TESTS_DIR/network.onnx" "$TESTS_DIR/inputs.npz" "$TESTS_DIR/outputs.npz"
+ rmdir "$TESTS_DIR" 2>/dev/null || true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| undo) | |
| echo "Undoing iLeakyReLU additions (file copies only)..." | |
| rm -rf "$TESTS_DIR" | |
| rm -f "$KERNEL_SRC_DIR/iLeakyReLU.c" | |
| rm -f "$KERNEL_INC_DIR/iLeakyReLU.h" | |
| rm -f "$TEMPLATES_DIR/iLeakyReLUTemplate.py" | |
| rm -f "$TILECONSTR_DIR/iLeakyReLUTileConstraint.py" | |
| echo "Note: hand-patches in Parsers.py / Bindings.py / Platform.py / Tiler.py / DeeployPULPMath.h were NOT removed." | |
| echo "If you need a fully clean tree, use: git checkout -- Deeploy/Targets TargetLibraries/PULPOpen/inc/DeeployPULPMath.h" | |
| undo) | |
| echo "Undoing iLeakyReLU additions (file copies only)..." | |
| rm -f "$TESTS_DIR/network.onnx" "$TESTS_DIR/inputs.npz" "$TESTS_DIR/outputs.npz" | |
| rmdir "$TESTS_DIR" 2>/dev/null || true | |
| rm -f "$KERNEL_SRC_DIR/iLeakyReLU.c" | |
| rm -f "$KERNEL_INC_DIR/iLeakyReLU.h" | |
| rm -f "$TEMPLATES_DIR/iLeakyReLUTemplate.py" | |
| rm -f "$TILECONSTR_DIR/iLeakyReLUTileConstraint.py" | |
| echo "Note: hand-patches in Parsers.py / Bindings.py / Platform.py / Tiler.py / DeeployPULPMath.h were NOT removed." | |
| echo "If you need a fully clean tree, use: git checkout -- Deeploy/Targets TargetLibraries/PULPOpen/inc/DeeployPULPMath.h" |
🤖 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 `@Tutorials/PartIII_solution/iLeakyReLU/deploy.sh` around lines 35 - 43, Update
the undo branch in deploy.sh to preserve pre-existing contents of TESTS_DIR:
remove only the copied iLeakyReLU test artifacts, and remove TESTS_DIR only when
it is empty. Keep the cleanup of the three iLeakyReLU source, header, and
template/constraint files unchanged.
| uint32_t per = (size + nC - 1) / nC; | ||
| // Round per-core chunk down to a multiple of 4 so the SIMD loop is | ||
| // tail-free. The Step 6a tile-size constraint already guarantees that | ||
| // size is a multiple of 16. | ||
| per &= ~0x3u; | ||
| uint32_t start = cid * per; | ||
| uint32_t end = (start + per > size) ? size : (start + per); | ||
|
|
||
| v4s *vIn = (v4s *)(pIn + start); | ||
| v4s *vOut = (v4s *)(pOut + start); | ||
| uint32_t nVec = (end - start) >> 2; | ||
|
|
||
| for (uint32_t i = 0; i < nVec; i++) { | ||
| v4s x = vIn[i]; | ||
| v4s s = x >> shift; // packed per-lane arith shift | ||
| vOut[i] = __builtin_pulp_max4(x, s); // max(x, x>>shift) = LeakyReLU | ||
| } | ||
|
|
||
| // Scalar tail (only when perf constraint is not installed) | ||
| for (uint32_t i = start + (nVec << 2); i < end; i++) { | ||
| int32_t xs = (int32_t)pIn[i]; | ||
| pOut[i] = (int8_t)((xs >= 0) ? xs : (xs >> shift)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix incomplete SIMD work partitioning. A permitted 16-element tile with eight cores computes per = 2, then Line 40 rounds it to zero, so no core writes output. More generally, rounding per-core work down loses the remainder; last-axis divisibility by 16 cannot guarantee flattened work is divisible into four-byte chunks for every core.
Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLU_simd.c#L36-L57: distribute completev4sblocks across cores, then have one core process the global scalar remainder.Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLUTileConstraint.py#L39-L51: remove the incorrect tail-free guarantee or enforce only constraints actually required after the SIMD partition is corrected.
📍 Affects 2 files
Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLU_simd.c#L36-L57(this comment)Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLUTileConstraint.py#L39-L51
🤖 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 `@Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLU_simd.c` around lines 36 -
57, Fix partitioning in Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLU_simd.c
lines 36-57 by distributing complete v4s blocks across cores without rounding
each core’s work to zero or discarding remainders, then have one core process
the global scalar remainder. Update
Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLUTileConstraint.py lines 39-51 to
remove the incorrect tail-free guarantee or enforce only constraints still
required by the corrected SIMD partition; use the existing SIMD loop and scalar
tail symbols as integration points.
There was a problem hiding this comment.
Are the student testing problem dimensions with remainders?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
Tutorials/PartIII_solution/iLeakyReLU/generate.py \
Tutorials/PartIII_solution/iLeakyReLU/README.md \
Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLU_simd.c \
Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLUTileConstraint.py \
Tutorials/PartIII_skeletons/iLeakyReLU/generate.py \
Tutorials/SoCDAML.md
do
echo "===== $f ====="
if [[ "$f" == *.py ]]; then
ast-grep outline "$f" --items all || true
fi
rg -n -C 3 'SHAPE|NUM_CORES|--cores|nC|per\s*=|iLeakyReLU|profileTiling|tile|divisible|shape' "$f" || true
doneLength of output: 12060
@Victor-Jung, not in the supplied full-tensor test: students generate a (1, 16, 64, 64) input (65,536 elements), and the documented SIMD run uses 8 cores. That gives 8,192 elements/core, which is divisible by 4.
However, the tiled run can invoke the kernel on individual tiles. The current constraint only makes the innermost tile dimension divisible by 16; it does not guarantee that every flattened tile has at least 32 elements or that its eight-way split is divisible by 4. So the standard artifact likely won’t reveal the bug, but a permitted 16-element tile would.
| def parseNode(self, node: gs.Node) -> bool: | ||
| wellFormed = all([ | ||
| len(node.inputs) == 1, | ||
| len(node.outputs) == 1, | ||
| 'mul' in node.attrs, | ||
| 'shift' in node.attrs, | ||
| ]) | ||
|
|
||
| if not wellFormed: | ||
| return False | ||
|
|
||
| self.operatorRepresentation['mul'] = int(node.attrs['mul']) | ||
| self.operatorRepresentation['shift'] = int(node.attrs['shift']) | ||
| return True |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 --glob '*.py' "iLeakyReLU|['\"]mul['\"]|['\"]shift['\"]" \
Tutorials Deeploy DeeployTestRepository: pulp-platform/Deeploy
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Candidate iLeakyReLU files:"
git ls-files 'Tutorials/PartIII_solution/iLeakyReLU/*' | sort
echo
echo "Relevant parser/template/kernel/tile generate:"
for f in \
Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLUParser.py \
Tutorials/PartIII_solution/iLeakyReLU/deploy.sh \
Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLUTemplate.py \
Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLUTileConstraint.py \
Tutorials/PartIII_solution/iLeakyReLU/generate.py
do
if [ -f "$f" ]; then
echo "--- $f"
cat -n "$f"
fi
done
echo
echo "Search for iLeakyReLU implementation/kernel references:"
rg -n --glob '*.[ch]' --glob '*.py' --glob '!Tutorials/PartIII_solution/iLeakyReLU/deploy.sh' "PULPiLeakyReLU|iLeakyReLU|leakyReLU|leaky" Tutorials/PartIII_solution/iLeakyReLU Tutorials/PartIII_skeletons Deeploy Targets 2>/dev/null || trueRepository: pulp-platform/Deeploy
Length of output: 26929
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Scalar kernel:"
cat -n Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLU.c
echo
echo "SIMD kernel:"
cat -n Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLU_simd.c
echo
echo "Header declaration:"
cat -n Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLU.h
echo
echo "Read-only verifier: compare scalar behavior to SIMD current implementation around mul/signals"
python3 - <<'PY'
from pathlib import Path
import re
simd_path = Path('Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLU_simd.c')
scalar_path = Path('Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLU.c')
simd = simd_path.read_text()
scalar = scalar_path.read_text()
simd_body_start = simd.find('{', simd.find('PULPiLeakyReLU_i8_i8'))
simd_body = simd[simd_body_start:] if simd_body_start != -1 else ''
scalar_body_start = scalar.find('{', scalar.find('PULPiLeakyReLU_i8_i8'))
scalar_body = scalar[scalar_body_start:] if scalar_body_start != -1 else ''
uses_mul = re.search(r'\bmul\b', simd_body) is not None
uses_shift = re.search(r'\bshift\b', simd_body) is not None
uses_packed_ops = bool(re.search(r'__builtin_pulp_vec_(sli|sri|and|unpack)|xpulp.*vec|Vec', simd_body))
scalar_uses_mul = re.search(r'\bmul\b', scalar_body) is not None
scalar_uses_shift = re.search(r'\bshift\b', scalar_body) is not None
print({
"scalar_uses_mul": scalar_uses_mul,
"scalar_uses_shift": scalar_uses_shift,
"simd_uses_mul": uses_mul,
"simd_uses_shift": uses_shift,
"simd_uses_packed_ops": uses_packed_ops,
})
# Simulate generated scalar/shift formula as described by generator for representative negative inputs.
def golden_for_case(x, mul, shift):
import numpy as np
pos = x.astype(np.int32)
neg = (mul * pos) >> shift
return np.clip(np.where(pos >= 0, pos, neg), -128, 127).astype(np.int8)
cases = [((1, 0), -1, 1), ((3, 3), -1, 1)]
print("formula examples neg_input=-1:")
for case, x, shape in cases:
for m, s in [(1,3),(2,3),(3,3)]:
print(case, "mul", m, "shift", s, "golden", int(golden_for_case(x, m, s)))
PYRepository: pulp-platform/Deeploy
Length of output: 5475
Reject iLeakyReLU parameters unsupported by the SIMD implementation. The parser accepts arbitrary mul/shift, but the SIMD path casts mul away and only applies max(x, x >> shift), so a node like mul=2, shift=3 produces wrong outputs under ./deploy.sh simd. Constrain the mapper/parser in both Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLUParser.py and the injected deploy.sh parser to the SIMD contract, e.g. mul == 1 and a supported non-negative shift, or implement SIMD multiplication.
🧰 Tools
🪛 Ruff (0.16.0)
[error] 22-22: Undefined name gs
(F821)
📍 Affects 2 files
Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLUParser.py#L22-L35(this comment)Tutorials/PartIII_solution/iLeakyReLU/deploy.sh#L98-L109
🤖 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 `@Tutorials/PartIII_solution/iLeakyReLU/iLeakyReLUParser.py` around lines 22 -
35, Constrain iLeakyReLU parsing to the SIMD contract in both
iLeakyReLUParser.parseNode and the injected parser in deploy.sh: accept only mul
== 1 and a supported non-negative shift, rejecting unsupported parameter
combinations before populating operatorRepresentation. Keep both parser
implementations consistent unless the SIMD implementation is updated to apply
multiplication.
Victor-Jung
left a comment
Victor-Jung
left a comment
There was a problem hiding this comment.
Very cool extension to the tutorial! It will be a very useful example to point at to ppl learning Deeploy. I have some comments before merging but we are in the right track.
There was a problem hiding this comment.
docs/_static dir already has this file?
| if getattr(args, 'enable_3x3', False): | ||
| gen_args_list.append("--enable-3x3") |
There was a problem hiding this comment.
--enable-3x3 is removed in #188 as 3x3 are now natively supported.
There was a problem hiding this comment.
docs/_static/tutorials/introduction dir already has this file
There was a problem hiding this comment.
docs/_static/tutorials/introduction dir already has this file
There was a problem hiding this comment.
docs/_static/tutorials/introduction dir already has this file
| | `generate.py` | Complete ONNX + golden-value generator | Run it (Step 1) | | ||
| | `iLeakyReLU.h` | Complete kernel header | Copy to `TargetLibraries/PULPOpen/inc/kernel/` (Step 3) | | ||
| | `iLeakyReLU.c` | Multi-core chunking provided; inner loop TODO | Fill the TODO, copy to `TargetLibraries/PULPOpen/src/` (Step 3) | | ||
| | `iLeakyReLU_simd.c` | SIMD chunking + load/max/store provided; one TODO line | Fill in Step 6b after the scalar works | |
| uint32_t per = (size + nC - 1) / nC; | ||
| // Round per-core chunk down to a multiple of 4 so the SIMD loop is | ||
| // tail-free. The Step 6a tile-size constraint already guarantees that | ||
| // size is a multiple of 16. | ||
| per &= ~0x3u; | ||
| uint32_t start = cid * per; | ||
| uint32_t end = (start + per > size) ? size : (start + per); | ||
|
|
||
| v4s *vIn = (v4s *)(pIn + start); | ||
| v4s *vOut = (v4s *)(pOut + start); | ||
| uint32_t nVec = (end - start) >> 2; | ||
|
|
||
| for (uint32_t i = 0; i < nVec; i++) { | ||
| v4s x = vIn[i]; | ||
| v4s s = x >> shift; // packed per-lane arith shift | ||
| vOut[i] = __builtin_pulp_max4(x, s); // max(x, x>>shift) = LeakyReLU | ||
| } | ||
|
|
||
| // Scalar tail (only when perf constraint is not installed) | ||
| for (uint32_t i = start + (nVec << 2); i < end; i++) { | ||
| int32_t xs = (int32_t)pIn[i]; | ||
| pOut[i] = (int8_t)((xs >= 0) ? xs : (xs >> shift)); |
There was a problem hiding this comment.
Are the student testing problem dimensions with remainders?
|
|
||
| void PULPiLeakyReLU_i8_i8(int8_t *pIn, int8_t *pOut, uint32_t size, int32_t mul, | ||
| int32_t shift) { | ||
| (void)mul; // SIMD path assumes mul == 1 |
There was a problem hiding this comment.
Why not apply the scale here? If there is a good reason the parser should test the mul value to be 1 and fail if it's not.
| - Forward `--neureka-wmem` and `--enable-3x3` from `deeployRunner` to the generator. Both were accepted by argparse and then dropped, so enabling the Neureka weight memory produced byte-identical cycle counts; `microLlama64` autoregressive now improves from 776k to 712k cycles with the flag | ||
| - Fix Part III `deploy.sh` half-applying its `Platform.py` patch: the parser-import replacement matched a single-line needle that no longer exists (the import block is line-wrapped), so the mapper was injected without its import and every PULP runner failed with `NameError: name 'iLeakyReLUParser' is not defined`. The patch is now anchored on the mapper definition and fails loudly if that anchor disappears | ||
| - Fix Part III Step 6a to use `addTileSizeDivisibleConstraint` instead of `addMinTileSizeConstraint`; the latter only forces the leftover tile to be at least `modulo` elements and gives no divisibility guarantee, so the documented multiple-of-16 SIMD alignment did not hold | ||
| - Fix Deeploy 101 and Part III tutorial errors: `--profileTiling` is a valueless flag (`--profileTiling=L2` aborts argparse), the intrinsics inventory path moved to `TargetLibraries/third_party/`, Step 2 needs `cd ../../../DeeployTest`, and the Step 4/5 solutions were missing their `Platform.py` imports |
There was a problem hiding this comment.
This is a bit too verbose for the changelogs, can you compress it?
| # ./deploy.sh # apply scalar kernel (Step 3) | ||
| # ./deploy.sh simd # apply SIMD kernel (Step 6b) on top | ||
| # ./deploy.sh undo # remove the additions (best-effort) | ||
| # ---------------------------------------------------------------------- |
There was a problem hiding this comment.
The patches to the Deeploy core library are very fragile and will break in the future. Could you provide these changes as git patches that can be applied with git apply so that if smth goes wrong ppl will get a merge conflict and can resolve it cleanly?
2 participants
This PR adds Part III to
Tutorials/SoCDAML.md, a 1-hour hands-on lab in which students add a new int8 operator (iLeakyReLU) to Deeploy and progressively optimize it on Siracusa from a plain-C scalar kernel running on a single core all the way to a tiled, multi-core, XPULP-SIMD kernel. The lab walks through the full compiler pipeline that Parts I and II used as a black box: Parser → Mapper → Binding → Template → TileConstraint → SIMD kernel. Students measure end-to-end cycles at every step and observe the speedup compared to the baseline implementation.Two coordinated artifact directories ship alongside the markdown:
Tutorials/PartIII_skeletons/iLeakyReLU/: student starting kit withTODO(student)markers and per-file destination paths.Tutorials/PartIII_solution/iLeakyReLU/: full TA reference (kernels, parser, template, tile constraint, ONNX + golden tensors) plus a one-shotdeploy.shthat patches the live source tree (Parsers.py,Bindings.py,Tiler.py,Platform.py,DeeployPULPMath.h) and copies the new kernel/template/constraint files into place. Re-runnable inscalar/simd/undomodes.Added
Tutorials/PartIII_skeletons/iLeakyReLU/: 8 skeleton files (generate.py, kernel + SIMD kernel skeletons, parser/template/tile-constraint skeletons, header,README).Tutorials/PartIII_solution/iLeakyReLU/: full working reference operator (parser, template, tile + perf constraint, scalar + SIMD kernels, header, ONNX + golden artifacts,deploy.sh,README).Tutorials/SoCDAML.mdwith 6 stepwise tasks and solution dropdowns matching Parts I/II.Changed
Tutorials/SoCDAML.md: appended Part III. Parts I and II are untouched.Fixed
PR Merge Checklist
develcommit and pointing todevel.CHANGELOG.mdfile has been updated.