es-ude / es-ude/OnDeviceTraining

arithmetic: enable TRACK_INSTRUCTIONS on the legacy op libs

Open
#351 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
1
Forks
3
Avg merge
1d 1h
Merged PRs (30d)
8

Description

## Summary

`ODT_TRACK_INSTRUCTIONS` (the CMake cache var gating per-element instruction
counters, `TRACK_INSTRUCTIONS` compile define) shipped for the new
`PointwiseFused` library (#328 groundwork) — `lerpFloat32TensorsInplace`,
`addcmulFloat32TensorsInplace`, `addcdivDenomFloat32TensorsInplace` each
increment a dedicated counter once per element, exposed via
`getLerpInstructionCounter`/`getAddcmulInstructionCounter`/
`getAddcdivDenomInstructionCounter` (`src/arithmetic/PointwiseFused.c`,
`src/arithmetic/include/PointwiseFused.h`).

The same `TRACK_INSTRUCTIONS` define already exists on two legacy
`src/arithmetic/` libraries — `Square.c` and `Matmul.c` — but neither is safe
to enable. This issue tracks fixing both and generalizing the counter
convention `PointwiseFused` established (per-element increments, a getter per
op, no shared global namespace collision) so the mechanism can be turned on
repo-wide.

## Verified blockers

1. **`Square.c:4` name mismatch — fails compilation under the define.**
```c
#ifdef TRACK_INSTRUCTIONS
#define SQUARE_FUNC_INT squareInt32WithInstructionCounter
```
No function named `squareInt32WithInstructionCounter` is defined anywhere
in the file. The actual counting function is `squareIntWithInstructionCounter`
(`Square.c:21`). `squareInt32(a)` (`Square.c:26-28`) calls
`SQUARE_FUNC_INT(a)`, which expands to the non-existent
`squareInt32WithInstructionCounter` — this fails to link (undefined
reference) the moment `TRACK_INSTRUCTIONS` is defined for this TU.

2. **Missing `getSquareInstructionCounter` declaration in `Square.h`.**
`Square.c:43` defines `getSquareInstructionCounter()`, but
`src/arithmetic/include/Square.h` only declares `squareInt32` and
`squareFloat32` — no external caller (including a test) can read the
counter without redeclaring it locally.

3. **No reset helper — counters are cumulative process-globals.**
`squareInstructionCounter` (`Square.c:15`) and `matmulInstructionCounter`
(`Matmul.c:24`) both persist across the whole test binary's lifetime with
no reset entry point. `PointwiseFused`'s tests work around this by
asserting the **delta** across a call (see
`test/unit/arithmetic/UnitTestPointwiseFused.c::testLerpCounterDeltaIsElementCount`
and siblings) rather than an absolute value — the same delta-based idiom
would be needed for Square/Matmul, or a reset function should be added.

4. **`Matmul` per-call vs. `PointwiseFused`/scalar per-element semantics —
inconsistent counting granularity, plus a SYM double-increment bug.**
`Square`'s counter increments once per **scalar call**
(`squareIntWithInstructionCounter`/`squareFloatWithInstructionCounter`,
`Square.c:21,34`) — consistent with per-element counting when the scalar
op is invoked in a loop. `Matmul`'s counter increments once per **whole
matmul call** (`matmulIntTensorsWithInstructionCounter`,
`matmulFloatTensorsWithInstructionCounter`, `Matmul.c:98-102,187-191`),
not per output element or per multiply-add — a fundamentally different
granularity that would need to be reconciled before the two libraries'
counters mean the same thing.

Worse, the SYM_INT32 path double-increments: `matmulSymIntTensorsWithInstructionCounter`
(`Matmul.c:222-232`) calls `matmulInt32Tensors(...)` (`Matmul.c:213`),
which — under `TRACK_INSTRUCTIONS` — dispatches through
`MATMUL_FUNC_INT` to `matmulIntTensorsWithInstructionCounter`
(`Matmul.c:104-106`), incrementing `matmulInstructionCounter` once
(`Matmul.c:101`); `matmulSymIntTensorsWithInstructionCounter` then
increments the *same* global a second time itself (`Matmul.c:231`). A
single SYM matmul call currently counts as 2 instructions instead of 1.

## Template to follow

`PointwiseFused` (#328) is the reference implementation for the pattern this
issue should generalize:
- the `define`/`ifdef` swap lives in the `.c` file only, keyed off a single
`TRACK_INSTRUCTIONS` macro compiled in via a per-library
`target_compile_definitions(... PUBLIC $<$:TRACK_INSTRUCTIONS>)`
(`src/arithmetic/CMakeLists.txt:238-239`), driven by the `ODT_TRACK_INSTRUCTIONS`
CMake cache var (`ON` in the `unit_test_debug`/`unit_test_asan` presets);
- one counter per operation (not one shared counter across a whole library);
- a getter per counter, matching the existing `getMatmulInstructionCounter`/
`getSquareInstructionCounter` naming convention;
- tests assert the **delta** across a call, since the counters are cumulative
process-globals with no reset.

## Scope

Not scoped here: whether Matmul's counter granularity should become
per-element (matching Square/PointwiseFused) or stay per-call with a
documented rationale — that's a design decision for whoever picks this up,
informed by what the eventual consumer (a resource/instruction estimator)
actually needs to measure.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Contributor guide

Open the contributing guide

Research direction

Read src/arithmetic/PointwiseFused.c and its header and tests first, then compare the TRACK_INSTRUCTIONS paths in src/arithmetic/Square.c, Square.h, Matmul.c, and Matmul.h. Run the arithmetic unit tests with ODT_TRACK_INSTRUCTIONS enabled. Done means both legacy libraries build under the define, expose usable counters, avoid the identified double increment, and have tests covering the chosen counting semantics.

Written by the indexing model from the issue text.

Assessment

Tech stack
c, cmake
Domain
build-system, embedded-iot, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.