Add double-precision float multiply example using PRU MAC hardware accelerator#119
Add double-precision float multiply example using PRU MAC hardware accelerator#119
Conversation
…h MAC hardware Co-authored-by: tleyrer <24540697+tleyrer@users.noreply.github.com>
Co-authored-by: tleyrer <24540697+tleyrer@users.noreply.github.com>
Review Summary by QodoAdd double-precision float multiply example using PRU MAC hardware accelerator
WalkthroughsDescription• Implements IEEE 754 double-precision (64-bit) float multiplication using PRU MAC hardware accelerator with optimized assembly achieving **85 cycles** (~425 ns @ 200 MHz) • Splits 53-bit significands into 21-bit and 32-bit halves, computes four 32×32→64 MAC partial products with on-the-fly carry accumulation into 106-bit result • Normalizes product via bit 105 detection and rounds to nearest using guard bit, with mantissa-overflow → exponent-increment handling • Includes C reference implementation (main_c_ref.c) using software FP library (__mpy2d) for comparison (~200+ cycles) • Provides comprehensive documentation covering IEEE 754 format, algorithm overview, register mapping, memory layout, and performance analysis • Adds complete build infrastructure with board-specific makefiles and CCS project specifications for am261x-lp/som, am263px-lp/cc, and am263x-lp/cc platforms • Includes linker command files, ROV configuration files, and project build wrappers for both PRU0 and PRU1 cores across all supported boards • **Limitations**: Normal numbers only; NaN, Infinity, denormals, and zero are not handled Diagramflowchart LR
A["53-bit × 53-bit<br/>Significands"] -->|"Split into<br/>21-bit + 32-bit"| B["Four 32×32<br/>MAC Operations"]
B -->|"Accumulate<br/>with carry"| C["106-bit<br/>Product"]
C -->|"Detect bit 105"| D["Normalize"]
D -->|"Guard bit<br/>rounding"| E["64-bit Result<br/>85 cycles"]
File Changes1. examples/double_float/firmware/main_c_ref.c
|
Code Review by Qodo
1. LBBO/SBBO lack bounds checks
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| LBBO &lo_A, r1, OFFS_A_LO, 4 | ||
| LBBO &hi_A, r1, OFFS_A_HI, 4 | ||
| LBBO &lo_B, r1, OFFS_B_LO, 4 | ||
| LBBO &hi_B, r1, OFFS_B_HI, 4 |
There was a problem hiding this comment.
1. lbbo/sbbo lack bounds checks 📘 Rule violation ⛨ Security
The new PRU firmware performs LBBO/SBBO memory accesses using offsets without any explicit bounds validation. This can lead to out-of-bounds reads/writes if the assumed PRU Data RAM layout/size is incorrect for a target configuration.
Agent Prompt
## Issue description
The firmware issues `LBBO`/`SBBO` reads/writes using offsets without any explicit bounds validation, violating the requirement that every PRU memory access be verified to stay within the intended region.
## Issue Context
These accesses assume PRU Data RAM base `r1=0` and fixed offsets (e.g., `OFFS_A_LO`, `OFFS_R_HI`), but there is no proof (check/assertion) that `offset + size` fits within the allocated Data RAM for the target.
## Fix Focus Areas
- examples/double_float/firmware/main.asm[135-138]
- examples/double_float/firmware/main.asm[317-318]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # which directories to search for assembly & C source files? | ||
| FILES_PATH := .. ../../.. . | ||
| vpath %.asm $(FILES_PATH) | ||
| vpath %.c $(FILES_PATH) | ||
| vpath %.cmd $(FILES_PATH) | ||
| vpath %.obj $(GEN_DIR) | ||
| C_FILES := $(notdir $(foreach directory, $(FILES_PATH), $(wildcard $(directory)/*.c))) | ||
| ASM_FILES := $(notdir $(foreach directory, $(FILES_PATH), $(wildcard $(directory)/*.asm))) | ||
|
|
||
| # Generate an object file for every *.asm and *.c source file in FILES_PATH | ||
| OBJECTS := $(patsubst %.asm, $(GEN_DIR)/%.obj, $(notdir \ | ||
| $(foreach directory, $(FILES_PATH), $(wildcard $(directory)/*.asm)))) | ||
| OBJECTS += $(patsubst %.c, $(GEN_DIR)/%.obj, $(notdir \ | ||
| $(foreach directory, $(FILES_PATH), $(wildcard $(directory)/*.c)))) |
There was a problem hiding this comment.
2. Duplicate main definition 🐞 Bug ✓ Correctness
The ti-pru-cgt makefiles auto-discover and compile all *.c under ../../.. (the firmware/ directory), which includes main_c_ref.c defining int main() while main.asm also defines main, causing a link-time multiple-definition failure for gmake all builds.
Agent Prompt
### Issue description
The PRU firmware makefiles auto-discover all `*.c` in `FILES_PATH := .. ../../.. .`. Since `../../..` resolves to `examples/double_float/firmware/`, the reference file `main_c_ref.c` (which defines `int main()`) is compiled and linked alongside `main.asm` (which also defines `main`), causing a duplicate-symbol link failure.
### Issue Context
`main_c_ref.c` is documented as a reference-only implementation and should not be part of the default firmware build.
### Fix Focus Areas
Update all board-specific firmware makefiles to ensure `main_c_ref.c` is excluded from `C_FILES`/`OBJECTS` (or relocate the file so it isn’t discovered):
- examples/double_float/firmware/am261x-lp/icss_m0_pru0_fw/ti-pru-cgt/makefile[15-28]
- examples/double_float/firmware/am261x-lp/icss_m0_pru1_fw/ti-pru-cgt/makefile[15-28]
- examples/double_float/firmware/am261x-som/icss_m0_pru0_fw/ti-pru-cgt/makefile[15-28]
- examples/double_float/firmware/am261x-som/icss_m0_pru1_fw/ti-pru-cgt/makefile[15-28]
- examples/double_float/firmware/am263px-lp/icss_m0_pru0_fw/ti-pru-cgt/makefile[15-28]
- examples/double_float/firmware/am263px-lp/icss_m0_pru1_fw/ti-pru-cgt/makefile[15-28]
- examples/double_float/firmware/am263px-cc/icss_m0_pru0_fw/ti-pru-cgt/makefile[15-28]
- examples/double_float/firmware/am263px-cc/icss_m0_pru1_fw/ti-pru-cgt/makefile[15-28]
- examples/double_float/firmware/am263x-lp/icss_m0_pru0_fw/ti-pru-cgt/makefile[15-28]
- examples/double_float/firmware/am263x-lp/icss_m0_pru1_fw/ti-pru-cgt/makefile[15-28]
- examples/double_float/firmware/am263x-cc/icss_m0_pru0_fw/ti-pru-cgt/makefile[15-28]
- examples/double_float/firmware/am263x-cc/icss_m0_pru1_fw/ti-pru-cgt/makefile[15-28]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
This feedback seems valid. Is there something in place to prevent the makefile from grabbing main_c_ref.c and using it as a build source?
There was a problem hiding this comment.
Ask❓
This feedback seems valid. Is there something in place to prevent the makefile from grabbing main_c_ref.c and using it as a build source?
Answer:
No—based on the new makefile logic shown in the diff, there is currently nothing preventing main_c_ref.c from being auto-discovered and compiled.
Why it gets picked up:
- The makefiles set
FILES_PATH := .. ../../.. . - With the makefile located at
examples/double_float/firmware/<board>/<core>/ti-pru-cgt/, the../../..entry resolves toexamples/double_float/firmware/ main_c_ref.clives exactly in that directory (examples/double_float/firmware/main_c_ref.c)C_FILESandOBJECTSare populated via$(wildcard $(directory)/*.c), somain_c_ref.cis included automatically.- At link time you then have two entry points:
main.asmdefinesmain:(exported with.global main)main_c_ref.cdefinesint main(void)
This will typically cause a “multiple definition ofmain” link error forgmake all.
The comment in main_c_ref.c (“NOT compiled into the default PRU firmware project”) may be true for CCS projects if the projectspec excludes it, but it is not true for these makefile builds as written.
Recommended mitigation (one of):
- Exclude the reference file in makefiles:
- e.g. after computing
C_FILES, doC_FILES := $(filter-out main_c_ref.c,$(C_FILES)) - and similarly remove from
OBJECTS(or deriveOBJECTSfrom filteredC_FILES/ASM_FILESrather than re-wildcarding).
- e.g. after computing
- Or move
main_c_ref.cinto a directory not searched byFILES_PATH(e.g.firmware/ref/), and only include it in a dedicated “C reference” build. - Or add a build switch (e.g.
BUILD_C_REF=y) that selects eithermain.asmormain_c_ref.c, but not both.
As-is, the bot’s feedback is valid for the makefile-based builds.
|
@tleyrer please resolve all comments to merge |
| @@ -0,0 +1,111 @@ | |||
| include ../../imports.mak | |||
There was a problem hiding this comment.
This project is not actually building because it was not added to the list in examples/makefile. Feedback #1: add project to examples/makefile and make sure it actually shows up in a top-level makefile build.
| @@ -0,0 +1,169 @@ | |||
| # Double Float Multiply (64-bit IEEE 754) using PRU MAC Hardware | |||
|
|
|||
There was a problem hiding this comment.
Feedback #2: please add this project to examples/readme.md. It needs both a 1 sentence description of the project, and to be added to the table under "Supported processors per-project"
|
|
||
| A companion C reference (`firmware/main_c_ref.c`) shows the equivalent code | ||
| that the C compiler produces, where the `*` operator on `double` values is | ||
| lowered to a call into the software FP runtime library (`__mpy2d`). |
There was a problem hiding this comment.
Feedback #3: Please add sections "supported combinations" and "validated HW & SW" to provide future customers with the "known good" reference point for when the project was last tested on hardware. You may use https://github.com/TexasInstruments/open-pru/tree/main/examples/fir as a template.
| * Copyright (C) 2024-2025 Texas Instruments Incorporated - http://www.ti.com/ | ||
| */ | ||
|
|
||
| /** |
There was a problem hiding this comment.
As per the AI feedback, I am concerned about this file getting automatically grabbed by the makefile structure, and then the build process not being able to tell whether it is supposed to build with this main function, or the one defined in the ASM file. I imagine it would be safer to change this from a .c file to a .txt file to ensure that the makefile doesn't grab it:
FILES_PATH := .. ../../.. .
vpath %.asm $(FILES_PATH)
vpath %.c $(FILES_PATH)
vpath %.cmd $(FILES_PATH)
vpath %.obj $(GEN_DIR)
C_FILES := $(notdir $(foreach directory, $(FILES_PATH), $(wildcard $(directory)/*.c)))
ASM_FILES := $(notdir $(foreach directory, $(FILES_PATH), $(wildcard $(directory)/*.asm)))
OBJECTS := $(patsubst %.asm, $(GEN_DIR)/%.obj, $(notdir \
$(foreach directory, $(FILES_PATH), $(wildcard $(directory)/*.asm))))
OBJECTS += $(patsubst %.c, $(GEN_DIR)/%.obj, $(notdir \
$(foreach directory, $(FILES_PATH), $(wildcard $(directory)/*.c))))````
manojKoppolu
left a comment
manojKoppolu
left a comment
There was a problem hiding this comment.
Nick comments need's to be resolved .
- renaming main_c_ref.c & updating documentation
Implements IEEE 754 double-precision (64-bit) float multiplication in optimized PRU assembly using the 32-bit MAC broadside accelerator, located at
examples/double_float/. A C reference (main_c_ref.c) using the software FP library path (__mpy2d) is included for comparison.Algorithm (
firmware/main.asm)p00,p01,p10,p11) with on-the-flyADD/ADCcarry accumulation into a 106-bit result{pp3:pp2:pp1}QBBS do_shift, pp3, 9(detects leading-1 at bit 104 vs 105)pp1, with mantissa-overflow → exponent-increment handlingPerformance
__mpy2d(software FP)Build infrastructure
Board-specific makefiles and projectspecs for am261x-lp/som, am263px-lp/cc, am263x-lp/cc — mirroring the existing
examples/structure with correctOPEN_PRU_PATHdepth (../../../../../..).Normal numbers only; NaN, Inf, denormals, and zero are not handled.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.