Skip to content

fix(codegen): align C++ emission with lowering and interpreter - #36

Merged
monatis merged 5 commits into
monatis:mainfrom
danielm5:bugfix/clip_codegen
Sep 25, 2026
Merged

monatis merged 5 commits into
monatis:mainfrom
danielm5:bugfix/clip_codegen

Conversation

@danielm5

Copy link
Copy Markdown
Collaborator

I used ggmlc.compile with the clip_vision model and it failed. After careful review, I've found a few issues which I fixed.

I've manually applied the code changes myself but I felt unit tests were probably missing so I asked AI to add coverage for the affected code. I hope that's acceptable for this project.

Summary of changes:

  • MUL_MAT: emit ggml_add for 3-input nodes, with a contiguity guard on the bias. Lowering produces [w, x, bias] for LINEAR-with-bias, which codegen previously dropped silently. Mirrors the interpreter's bias path.
  • REPEAT: handle single-input nodes (e.g. EXPAND from the PyTorch importer) via ggml_repeat_4d with output dims.
  • RESHAPE: materialize non-contiguous inputs with ggml_cont before ggml_reshape_4d, as GET_ROWS/FLASH_ATTN already do.
  • VIEW: compute the byte offset as start * nb[ggml_dim] from the SLICE lowering attributes; the old "offset" attribute had no producer and always evaluated to 0.
  • PERMUTE: read axis0..axis3 scalars, the single form shared by the GGUF spec and the C++ interpreter.
  • lowering: remove unreachable SLICE/CONCAT branches.
  • tests: cover lowering and codegen emission for the above.

- MUL_MAT: emit ggml_add for 3-input nodes, with a contiguity
  guard on the bias. Lowering produces [w, x, bias] for
  LINEAR-with-bias, which codegen previously dropped silently.
  Mirrors the interpreter's bias path.
- REPEAT: handle single-input nodes (e.g. EXPAND from the
  PyTorch importer) via ggml_repeat_4d with output dims.
- RESHAPE: materialize non-contiguous inputs with ggml_cont
  before ggml_reshape_4d, as GET_ROWS/FLASH_ATTN already do.
- VIEW: compute the byte offset as start * nb[ggml_dim] from
  the SLICE lowering attributes; the old "offset" attribute
  had no producer and always evaluated to 0.
- PERMUTE: read axis0..axis3 scalars, the single form shared
  by the GGUF spec and the C++ interpreter.
- lowering: remove unreachable SLICE/CONCAT branches.
- tests: cover lowering and codegen emission for the above.
@danielm5

Copy link
Copy Markdown
Collaborator Author

I want to add tests that actually compile and run the generated C++ code rather than looking at the generated string. I'm doing a small try to validate this idea.

- Removed the previously added string comparison tests
- Added standalone tests that compile the generated code, run the
  program, and verify numeric results
…erpreter

- SQRT: emit ggml_sqrt with cont guard; handle is_rsqrt via
  div(sqrt(x), x). Mirrors the interpreter's sqrt path.
- SUM_ROWS: emit ggml_sum_rows with ggml_dim handling
  (transpose/reduce/transpose for dim 1, flatten for dim 2+)
  and reshape to the lowered output shape.
- ADD/SUB/MUL/DIV: route through a generated match_broadcast
  helper mirroring the interpreter, so keepdim broadcast
  (e.g. x / norm) works in standalone builds.
- tests: standalone l2_normalize (x / norm(keepdim)) matching
  torch numerically.
@monatis

monatis commented Sep 25, 2026

Copy link
Copy Markdown
Owner

Hi @danielm5 good catch, thanks for the fix! I'll merge it after completion of the CI tets

@monatis

monatis commented Sep 25, 2026

Copy link
Copy Markdown
Owner

I've just enabled tests on Windows after verifying them locally.

@monatis
monatis merged commit 6e54f69 into monatis:main Sep 25, 2026
10 checks passed
@danielm5

Copy link
Copy Markdown
Collaborator Author

Thank you so much!

As I was trying to use it to make a standalone version of Clip, I've found a few problems:

  1. Failed to generate code for this model
  2. Missing operations on the generated graph (after I fixed 1)
  3. C++ compiler errors on generated project due to model including "stdlib_kernels.h"
  4. C++ standalone program errors on execution as the generated main function uses a generic input rather than the actual compiled model one

I've fixed 1) and 2) for ClipVision but if we try other models there could be additional errors. For 3) and 4) I had to manually edit the generated files before using them. I might try to create patches for them too if you agree.

Finally, I was trying to understand more about the project. It appears to me these problems are due to the C++ generation code lagging behind from edits to the runtime, right? I'm not sure how, but it'd be nice if we had a better way to keep them in sync.

@monatis

monatis commented Sep 25, 2026

Copy link
Copy Markdown
Owner

Hi @danielm5, yes exactly. I implemented the codegen at the very early stage to prove myself that it's doable, but later I focused more on the compiler logic and generic runtime after I exchanged some emails with Ghorgi Gerganov (I optimized auto-regressive inference in runtime and made some comparative benchmarks vs. llama.cpp.) That's why the codegen lagged a little behind the rest of the project. I find it quite promising, though. Especially application-level details and custom kernel optimizations could be achieved with that path. So any contribution is appreciated and more than welcome.

I'm not sure how, but it'd be nice if we had a better way to keep them in sync.

Agree. We need to share the logic as much as possible. Currently codegen and compile functions are not well-engineered --their common logic needs to be extracted to a helper function to prevent any such divergence in the future. I'm also not happy with their loosely typed arguments (like fuse options typed a dictionary etc.).

ggmlc first started as an experment of mine, but seems like it'll be really useful, so these parts deserve a rework.

@danielm5
danielm5 deleted the bugfix/clip_codegen branch September 25, 2026 18:28
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