Skip to content

Simplify fkl-python: generic IO IOps + let C++ fuse_back fuse (Oscar review) - #1

Merged
johnnynunez merged 3 commits into
mainfrom
refactor/generic-io-ops-and-cpp-fusion
Jun 24, 2026
Merged

Simplify fkl-python: generic IO IOps + let C++ fuse_back fuse (Oscar review)#1
johnnynunez merged 3 commits into
mainfrom
refactor/generic-io-ops-and-cpp-fusion

Conversation

@johnnynunez

@johnnynunez johnnynunez commented Jun 24, 2026

Copy link
Copy Markdown
Owner

Contexto


Punto 1 — IO genérica en los descriptores de IOp

generate_cu() construía el buffer de entrada/salida con una escalera if/elif cerrada keyed en op.name, que solo cubría un puñado de ops de memoria y duplicaba lógica que pertenece a la propia IOp.

  • Cada Read op (_ReadOp) implementa ahora emit_read(state, mem, n_inputs) -> (in_decl, read_expr): construye su propio objeto de buffer host (Ptr2D / Tensor / std::array<Ptr2D,B> / TensorPack / ReadSet) y el build() que lo consume. Una Read IOp es autocontenida, igual que en C++ el buffer viaja dentro del tipo de la IOp.
  • Cada Write op (_WriteOp) implementa emit_write(state, mem, pbase) (TensorWrite / TensorSplit / TensorTSplit / SplitWrite).
  • generate_cu() ya no tiene escalera: solo llama read_op.emit_read() / write_op.emit_write().
  • Se eliminan los cuerpos muertos de TensorRead/Write.cpp() (in_tensor/out_tensor, input.ptr()) que el codegen sobreescribía y nunca llamaba.

Verificación: el C++ emitido es byte-idéntico a main en las 9 variantes de IO (Ptr2D, Tensor-planes, TensorSplit, TensorTSplit, SplitWrite, TensorPack, ReadSet, BorderReader+Crop, batch Crop) × {gpu, cpu}. Como el output no cambia, la caché de compilación se reutiliza (sin recompile storm) y no hace falta bump de CODEGEN_VERSION.


Punto 2 — Dejar que C++ fuse_back fusione

executeOperations(stream, iOps...) ya llama internamente a BackFuser::fuse_back(iOps...) (executors.h). El build() sin valor de BorderReader devuelve un IncompleteReadBack, que fuse_back detecta y fusiona con la Read previa vía fk::fuse. Python splicear la expresión de read dentro del BorderReader estaba reimplementando una fusión que C++ hace gratis.

  • replicate / reflect / wrap / reflect101 → ahora emiten una IOp IncompleteReadBack plana en la lista y la librería las fusiona. Verificado numéricamente (replicate + crop OOB sigue casando con la referencia CPU, sin splice en Python).
  • Excepción honesta — CONSTANT: el builder incompleto BorderReader<CONSTANT>::build(value) no compila upstream (border_reader.h:72 pasa NullType donde se requiere un backIOp; reproducido en aislado con nvcc). La FKL solo soporta la forma completa build(readIOp, value). Para ese único modo se mantiene un splice mínimo (marcador _needs_read), claramente acotado y documentado. Todos los demás modos van por el camino limpio de fuse_back.

CODEGEN_VERSION 7 → 8 (cambia el C++ emitido de los borders sin valor).

Nota sobre lo que no se puede simplificar (honestidad): el threading de tipos en Python (Mul<float3>, Cast<float3,uchar3>, …) es load-bearing, no redundante. No existe ningún Mul<>::build deducido en toda la FKL — los Binary ops exigen <T> explícito. También batch_fuse_head para batch Crop se mantiene: devuelve Read<BatchRead> tipado como ReadType, que fuse_back::idxFirstNonBack no detecta (necesita el fk::fuse explícito). Verificado en el código.


Tests (venv del proyecto, GPU RTX PRO 6000 sm_120, nvcc 13.3)

Verde: operations, vertical/horizontal fusion, niche (ambos borders), roi, e2e, circular, dlpack, torch, flash_attention (23), cpu_backend, matrix, thread_fusion.

Las 3 fallas de test_batch_divergent_hf (DivergentHF: …) son pre-existentes en main (bug de selector upstream DivergentBatchTransformDPP, camino de código distinto, no tocado en este PR). Idénticas antes y después.

El tercer commit actualiza la skill fkl-python-extending (arquitectura emit_read/emit_write + el matiz de fusión de BorderReader).

Point 1 (Oscar review): the initial/final IOps were special-cased by a
closed if/elif ladder in generate_cu() keyed on op .name, which silently
only covered a handful of memory ops and duplicated logic that belongs on
the IOp itself. Move each read/write op's host-side buffer construction
into emit_read()/emit_write() on the descriptor, so a Read/Write IOp is
self-contained (mirroring how the C++ side carries the buffer inside the
IOp type). generate_cu now just calls read_op.emit_read()/write_op.emit_write().

Also drops the dead TensorRead/Write.cpp() bodies (in_tensor/out_tensor /
input.ptr()) that codegen overrode and never called.

Verified emitted C++ is byte-identical to HEAD across all 9 IO variants
(Ptr2D, Tensor-planes, TensorSplit, TensorTSplit, SplitWrite, TensorPack,
ReadSet, BorderReader+Crop, batch Crop) x {gpu,cpu}. Full suite green:
operations, vertical/horizontal fusion, niche, roi, e2e, circular, dlpack,
torch. No CODEGEN_VERSION bump needed (output unchanged -> cache reused).
…splice)

Point 2 (Oscar review): executeOperations() already runs
BackFuser::fuse_back(iOps...) internally (executors.h), and BorderReader's
value-less build() returns an IncompleteReadBack that fuse_back detects and
fuses with the preceding Read via fk::fuse. So Python splicing the read
expression into BorderReader was reimplementing fusion C++ does for free.

Now replicate/reflect/wrap/reflect101 emit a plain IncompleteReadBack IOp in
the flat list and let the library fuse it — verified numerically (niche
replicate+OOB-crop still matches the CPU reference, with NO Python splice).

CONSTANT stays on the read-splice path: FKL's incomplete-const builder
BorderReader<CONSTANT>::build(value) does NOT compile (border_reader.h:72
passes NullType where a backIOp is required; reproduced in isolation), so the
library only supports the complete build(readIOp, value) form. Kept a minimal
_needs_read splice for that single mode, clearly scoped and documented.

Renamed the generic _fuse_with_read marker to _needs_read to reflect that it
is now a narrow FKL-limitation workaround, not the general border path.
CODEGEN_VERSION 7 -> 8 (emitted C++ for value-less borders changed).
@johnnynunez
johnnynunez merged commit 9cbbccb into main Jun 24, 2026
5 checks passed
@johnnynunez

Copy link
Copy Markdown
Owner Author

Extra: arreglados los 3 fallos pre-existentes de DivergentHF

Aprovechando el PR, arreglé también las 3 fallas de test_batch_divergent_hf (DivergentHF: …) que venían de main y había marcado como fuera de alcance.

Causa: off-by-one en el selector de secuencias. DivergentBatchTransformDPP es 0-basedexec() llama a divergent_operate<0>(z, seqs...) y ejecuta la secuencia cuya posición 0-based coincide con at(z) (data_parallel_patterns.h; el selector del test de regresión upstream devuelve index==0?0u:1u, o sea 0 elige la PRIMERA secuencia). _selector_cpp emitía el plane_map 1-based tal cual, así que at(0)=1 seleccionaba la SEGUNDA secuencia para el plano 0. Por eso el plano 0 ejecutaba siempre la seq equivocada.

  • compose_divergent mantiene el plane_map 1-based en la API (legible); el selector ahora emite (s-1).
  • El comentario que citaba circular_tensor.h::SequenceSelectorType como "1-based convention" era incorrecto para este kernel (es otro contrato de selector) — corregido.
  • Añadido sv=2 a la firma de caché del divergente para no reusar .so obsoletas del selector buggeado.

test_batch_divergent_hf ahora 12/12 (antes 9/12). circular_tensor (que usa el otro selector), HF y e2e siguen verdes. Suite completa sin fallos.

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.

1 participant