Skip to content

ggml-tsavorite: don't report OPU Target for ops with no kernel invocation - #160

Closed
akapoor3518 wants to merge 1 commit into
masterfrom
fix-opu-mislabel-no-kernel
Closed

akapoor3518 wants to merge 1 commit into
masterfrom
fix-opu-mislabel-no-kernel

Conversation

@akapoor3518

@akapoor3518 akapoor3518 commented Aug 18, 2026 •

Copy link
Copy Markdown

Summary

Found while reviewing GGML Perf Summary output enabled by FIR-2216/PR #158. Some ops with
TSI_KERNEL-RUN == 0 kernel launches were still labeled "OPU" in the Target column, which reads
as if they ran on Tsavorite hardware when they were actually computed via a direct call to
ggml's own CPU compute function.

Example (before this fix):

  Op               Target      Runs  TSI_KERNEL-RUN          Total us            Avg us
  SET_ROWS         CPU          728               0              4799              6.59
  GET_ROWS         OPU           15               0               369             24.60
  SOFT_MAX         OPU          110               0             78947            717.70
  ROPE             CPU          716               0             10128             14.15

SET_ROWS and ROPE correctly showed CPU, but GET_ROWS and SOFT_MAX -- going through the exact
same "no blob, call the plain CPU compute function directly" pattern in the dispatch switch --
showed OPU instead, despite also having 0 kernel runs.

Root cause

The per-node compute loop unconditionally marked every node it processed as
GGML_COMPUTE_BACKEND_TSAVORITE at the end of each iteration, regardless of whether a kernel
actually launched or the node was computed via the CPU-fallback path
(ggml_compute_forward_get_rows/ggml_compute_forward_soft_max/etc., called directly a few
lines earlier for op types with no kernel implementation).

ggml_tsavorite_internal_supports_op() accepts these op types identically (unconditional
return true for GET_ROWS, SOFT_MAX, SET_ROWS, ROPE, etc.), so the inconsistency in the example
above isn't explained there -- it's specifically the backend-label assignment at the end of the
compute loop.

Fix

Snapshot tsi_kernel_runs at the start of each node's processing and only report
GGML_COMPUTE_BACKEND_TSAVORITE if it actually increased during this node's compute; otherwise
report GGML_COMPUTE_BACKEND_CPU.

Validation

Verified via ollama (Makefile.sync temporarily pointed at this branch, merged with the
FIR-2238 perf_reset fix for a combined test): GET_ROWS, SOFT_MAX, and CONT (also affected, same
pattern) now correctly show CPU instead of OPU. ADD/MUL/RMS_NORM/MUL_MAT/GLU -- which do launch
real kernels -- are unaffected and still correctly show OPU. Confirmed the underlying
TSI_KERNEL-RUN magnitude for the reclassified ops is unchanged before/after this fix (this fix
only changes which Target bucket the existing numbers land in, not the numbers themselves).


Summary by cubic

Fixes mislabeling in GGML Perf Summary: nodes with zero TSI_KERNEL-RUN are no longer reported as OPU. Old behavior marked all processed nodes as OPU; new behavior reports OPU only if a kernel launched, otherwise CPU. This changes reporting only.

  • Captures tsi_kernel_runs at node start and sets node->ggml_compute_backend to TSAVORITE only if it increases; otherwise sets CPU.
  • Applies to ops without kernels (e.g., GET_ROWS, SOFT_MAX, CONT); kernel-backed ops remain OPU.
  • No change to execution path, run counts, or timings; only the Target classification is corrected.

Written for commit 0960023. Summary will update on new commits.

Review in cubic

…tion

The per-node compute loop unconditionally marked every node it processed
as GGML_COMPUTE_BACKEND_TSAVORITE, even for op types with no kernel/blob
(GET_ROWS, SOFT_MAX, etc.), which are instead computed by calling ggml's
own CPU compute function directly. This made the GGML Perf Summary's
Target column report "OPU" for ops that had zero kernel runs and were
actually computed on CPU, misleading anyone reading it to understand
what's actually running on hardware.

Snapshot tsi_kernel_runs at the start of each node's processing and only
report GGML_COMPUTE_BACKEND_TSAVORITE if it actually increased; otherwise
report GGML_COMPUTE_BACKEND_CPU, matching what ops like SET_ROWS/ROPE
already correctly showed when routed the same way.
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Correct Tsavorite perf targets for CPU-fallback operations

🐞 Bug fix 🕐 10-20 Minutes

Grey Divider

AI Description

• Classifies Tsavorite-dispatched nodes using actual kernel launches instead of backend ownership.
• Reports CPU fallback operations as CPU while preserving OPU labels for launched kernels.
Diagram

graph TD
    A["GGML Node"] --> B["Snapshot Count"] --> C["Dispatch Compute"] --> D{"Count Increased?"} -->|Yes| E["OPU Target"]
    D -->|No| F["CPU Target"]
Loading
High-Level Assessment

The counter-delta approach is appropriate because tsi_kernel_runs is already updated at the actual kernel launch sites and provides a common signal across all dispatch branches. An explicit per-path boolean was considered, but it would duplicate launch tracking and be easier to omit when new kernels or fallback paths are added.

Files changed (1) +14 / -1

Bug fix (1) +14 / -1
ggml-tsavorite.cppDerive perf backend labels from per-node kernel activity +14/-1

Derive perf backend labels from per-node kernel activity

• Snapshots each node's TSI kernel-run count before computation and compares it afterward during perf accounting. Nodes that launched kernels remain classified as Tsavorite/OPU, while direct GGML CPU fallback operations are classified as CPU.

ggml/src/ggml-tsavorite/ggml-tsavorite.cpp

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

Re-trigger cubic

// exactly what the Target/TSI_KERNEL-RUN columns are meant to make visible.
node->ggml_compute_backend = (node->tsi_kernel_runs > tsi_kernel_runs_before_node)
? GGML_COMPUTE_BACKEND_TSAVORITE
: GGML_COMPUTE_BACKEND_CPU;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the way we used to display was everything as OPU but the txe_kernel run being counted only when txe executes it otherwise it is empty. Is it the case even now ?

@akapoor3518

Copy link
Copy Markdown
Author

Merged into #159 -- closing this in favor of the combined PR.

@akapoor3518
akapoor3518 deleted the fix-opu-mislabel-no-kernel branch August 18, 2026 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants