Skip to content

Commit fdb9362

Browse files
committed
fix(path-c): close topk_selector masked-interval scaffold + fix owner-output route bug
topk_selector masked intervals fell back to the pure-MLX reference. Two blockers (NOT a codegen gap - the masked prim topk_selector_kernel already applies the mask in-kernel via valid=(j>=s)&(j<e)): - an artificial guard raised TopKPathCDirectError for any masked call; - a pre-existing, mask-independent bug: the `returned is not indices` identity check broke the ENTIRE owner-output route (masked + unmasked) on the installed TileLang, because tvm-ffi out_idx=1 returns a fresh MLX wrapper around the caller buffer, not the literal object (confirmed failing on pristine origin/main). Fix: remove the masked guard; relax the ownership check to validate + mirror the returned int32 (B,k) wrapper into the caller-owned buffer; pass starts/ends through AUTO + explicit dispatch. Metal: masked AUTO parity 10/10 set-equal to reference (5 shapes x fp32/fp16), all via Path C (0 ref fallbacks); short/empty -> -1 sentinels. No irreducible sub-case. No TileLang C++ change / no rebuild. Tests 96 passed (1 pre-existing brittle string-match fail confirmed on pristine); Path A/B + static-interval path unchanged.
1 parent 9c7d14f commit fdb9362

2 files changed

Lines changed: 128 additions & 69 deletions

File tree

cppmega_mlx/nn/_tilelang/topk_selector.py

Lines changed: 55 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -60,12 +60,15 @@
6060
Performance vs MLX's built-in ``argpartition``
6161
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
6262
63-
MLX's ``mx.argpartition`` is itself an optimised Metal kernel. Unmasked AUTO
64-
routes now use the TileLang/tvm-ffi owner-output path when it is available;
65-
masked intervals fall back to the pure-MLX reference until the owner-output
66-
Path C route grows masked-row parity. Explicit ``backend="metal"`` is retained
67-
only as a fail-closed compatibility spelling for callers that still probe the
68-
retired Path B surface.
63+
MLX's ``mx.argpartition`` is itself an optimised Metal kernel. Both unmasked
64+
and masked AUTO routes now use the TileLang/tvm-ffi owner-output path when it is
65+
available: the Path C prim reads ``starts``/``ends`` and masks columns outside
66+
``[start, end)`` to ``-inf`` before the per-lane top-K insertion, so masked rows
67+
reach set-equality parity with the pure-MLX reference (including the ``-1``
68+
sentinel for intervals shorter than ``k``). The pure-MLX reference is used only
69+
as a fallback when Path C cannot dispatch. Explicit ``backend="metal"`` is
70+
retained only as a fail-closed compatibility spelling for callers that still
71+
probe the retired Path B surface.
6972
7073
Algorithm sketch::
7174
@@ -725,12 +728,12 @@ def topk_selector_tilelang_direct(
725728
raise ValueError(
726729
f"topk_selector expects a (B, T) array; got shape {scores.shape}"
727730
)
728-
if starts is not None or ends is not None:
729-
raise TopKPathCDirectError(
730-
"topk_selector_tilelang direct tvm-ffi route is currently "
731-
"limited to unmasked rows; masked start/end intervals remain on "
732-
"Path B until the TileLang owner-output lowering is parity-clean"
733-
)
731+
# Masked start/end intervals are handled directly by the Path C prim:
732+
# the kernel reads ``starts[bx]``/``ends[bx]`` and masks columns outside
733+
# ``[start, end)`` to ``-inf`` before the per-lane top-K insertion, so the
734+
# owner-output route is parity-clean for masked rows (set-equality with the
735+
# pure-MLX reference, including the ``-1`` sentinel when an interval yields
736+
# fewer than ``k`` valid columns). No separate masked prim is required.
734737
batch = int(scores.shape[0])
735738
seq_len = int(scores.shape[1])
736739
if k < 1 or k > seq_len:
@@ -779,14 +782,29 @@ def topk_selector_tilelang_direct(
779782
raise TopKPathCDirectError(
780783
f"direct tvm-ffi topk dispatch failed: {type(exc).__name__}: {exc}"
781784
) from exc
782-
if returned is not indices:
783-
raise TopKPathCDirectError(
784-
"direct tvm-ffi topk did not return the caller-owned output"
785-
)
786785
# TVM encodes into MLX's current Metal command buffer outside the MLX graph.
787786
# The returned owner array has no lazy MLX dependency edge, so force the
788787
# command buffer to complete before Python or downstream consumers observe it.
789788
mx.synchronize()
789+
if returned is indices:
790+
# tvm-ffi handed back the exact caller-owned object.
791+
return indices
792+
# Some installed TileLang/tvm-ffi builds return a fresh MLX wrapper around
793+
# the caller's ``out_idx`` buffer instead of the literal Python object. The
794+
# kernel still wrote into the caller's storage (out_idx=1 == ``indices``),
795+
# so the ownership contract holds. Validate the wrapper is a compatible
796+
# int32 (batch, k) array and mirror its values into the caller-owned buffer
797+
# so downstream consumers see the result on the exact object they passed.
798+
if (
799+
not isinstance(returned, mx.array)
800+
or tuple(returned.shape) != tuple(indices.shape)
801+
or returned.dtype != indices.dtype
802+
):
803+
raise TopKPathCDirectError(
804+
"direct tvm-ffi topk did not return the caller-owned output"
805+
)
806+
indices[:, :] = returned
807+
mx.eval(indices)
790808
return indices
791809

792810

@@ -843,10 +861,9 @@ class PathBStatus:
843861

844862

845863
_PATH_B_RETIRED_REASON = (
846-
"topk_selector direct-MSL Path B is retired. Unmasked AUTO calls should use "
847-
"the TileLang/tvm-ffi owner-output route; masked or unsupported no-output "
848-
"calls fall back to the pure-MLX reference until masked owner-output Path C "
849-
"parity lands."
864+
"topk_selector direct-MSL Path B is retired. Both unmasked and masked AUTO "
865+
"calls use the TileLang/tvm-ffi owner-output route; only calls where Path C "
866+
"cannot dispatch fall back to the pure-MLX reference."
850867
)
851868

852869

@@ -881,11 +898,11 @@ def topk_selector(
881898
starts: optional (B,) int32 starts for per-row mask.
882899
ends: optional (B,) int32 ends for per-row mask.
883900
backend: ``"auto"`` (default) tries TileLang Path C with an owner-output
884-
buffer for unmasked calls, then falls back to the pure-MLX reference;
885-
``"metal"`` fails closed because Path B is retired;
886-
``"tilelang"`` / ``"path_c"`` require the owner-output Path C
887-
helper and allocate the public result buffer for unmasked calls;
888-
``"mlx"`` always uses the reference.
901+
buffer for both unmasked and masked (start/end interval) calls, then
902+
falls back to the pure-MLX reference; ``"metal"`` fails closed because
903+
Path B is retired; ``"tilelang"`` / ``"path_c"`` require the
904+
owner-output Path C helper and allocate the public result buffer for
905+
unmasked and masked calls; ``"mlx"`` always uses the reference.
889906
"""
890907

891908
if backend not in {"auto", "mlx", "metal", "tilelang", "path_c"}:
@@ -897,15 +914,21 @@ def topk_selector(
897914
if backend in {"tilelang", "path_c"}:
898915
try:
899916
owner_out = mx.zeros((int(scores.shape[0]), int(k)), dtype=mx.int32)
900-
return topk_selector_tilelang_direct(scores, k, out=owner_out)
917+
return topk_selector_tilelang_direct(
918+
scores, k, starts=starts, ends=ends, out=owner_out
919+
)
901920
except TopKPathCDirectError as exc:
902921
raise RuntimeError("topk_selector: TileLang Path C path unavailable") from exc
903-
if starts is None and ends is None:
904-
try:
905-
owner_out = mx.zeros((int(scores.shape[0]), int(k)), dtype=mx.int32)
906-
return topk_selector_tilelang_direct(scores, k, out=owner_out)
907-
except TopKPathCDirectError:
908-
pass
922+
# AUTO routes both unmasked and masked intervals through the owner-output
923+
# Path C prim (the kernel applies the [start, end) mask itself); any
924+
# dispatch/compile failure falls back to the pure-MLX reference.
925+
try:
926+
owner_out = mx.zeros((int(scores.shape[0]), int(k)), dtype=mx.int32)
927+
return topk_selector_tilelang_direct(
928+
scores, k, starts=starts, ends=ends, out=owner_out
929+
)
930+
except TopKPathCDirectError:
931+
pass
909932
return topk_selector_reference(scores, k, starts=starts, ends=ends)
910933

911934

tests/test_tilelang_topk.py

Lines changed: 73 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -537,9 +537,10 @@ def test_path_c_direct_tvm_ffi_reuses_owner_output_and_mutates_buffer() -> None:
537537
assert _to_index_sets(out) == _to_index_sets(out_ref)
538538

539539

540-
def test_path_c_direct_rejects_starts_ends_mask(
541-
monkeypatch: pytest.MonkeyPatch,
542-
) -> None:
540+
def test_path_c_direct_masked_interval_parity_set_equality() -> None:
541+
"""Masked start/end intervals now go through the owner-output Path C prim
542+
(no fallback to the pure-MLX reference) and match it for set membership."""
543+
543544
rng = np.random.default_rng(20260504)
544545
batch, seq_len, k = 3, 64, 4
545546
scores_np = rng.standard_normal((batch, seq_len)).astype(np.float32)
@@ -552,45 +553,50 @@ def test_path_c_direct_rejects_starts_ends_mask(
552553
scores = mx.array(scores_np)
553554
starts = mx.array(starts_np)
554555
ends = mx.array(ends_np)
555-
out = mx.full((batch, k), -123, dtype=mx.int32)
556556

557-
monkeypatch.setattr(
558-
topk_selector_mod,
559-
"_path_c_tvm_ffi_kernel_for",
560-
lambda *_, **__: (_ for _ in ()).throw(
561-
AssertionError("masked direct Path C must fail before compile")
562-
),
563-
)
557+
out_c = _topk_tilelang_direct_output(scores, k, starts=starts, ends=ends)
558+
out_ref = topk_selector_reference(scores, k, starts=starts, ends=ends)
559+
mx.eval(out_c, out_ref)
564560

565-
with pytest.raises(TopKPathCDirectError, match="limited to unmasked rows"):
566-
topk_selector_tilelang_direct(scores, k, starts=starts, ends=ends, out=out)
567-
mx.eval(out)
568-
assert np.asarray(out).tolist() == [[-123] * k] * batch
561+
assert _to_index_sets(out_c) == _to_index_sets(out_ref)
562+
assert out_c.dtype == mx.int32
563+
assert tuple(out_c.shape) == (batch, k)
564+
# Every selected index lies inside its row's [start, end) interval.
565+
arr = np.asarray(out_c)
566+
for b in range(batch):
567+
for idx in arr[b]:
568+
if idx >= 0:
569+
assert starts_np[b] <= idx < ends_np[b]
569570

570571

571572
@pytest.mark.parametrize(
572573
"starts_np,ends_np",
573574
[
574575
(np.array([0, 3], dtype=np.int32), None),
575576
(None, np.array([-1, 99], dtype=np.int32)),
577+
(np.array([1, 2], dtype=np.int32), np.array([3, 2], dtype=np.int32)),
576578
],
577579
)
578-
def test_path_c_direct_rejects_partial_interval_mask(
580+
def test_path_c_direct_partial_interval_mask_parity(
579581
starts_np: np.ndarray | None,
580582
ends_np: np.ndarray | None,
581583
) -> None:
584+
"""Partial (starts-only / ends-only), out-of-range, and short/empty
585+
intervals all reach set-equality parity with the reference through the
586+
owner-output Path C prim, including the ``-1`` sentinel for short rows."""
587+
582588
scores = mx.array(np.array([
583589
[5.0, 4.0, 3.0, 2.0],
584590
[0.0, 1.0, 2.0, 3.0],
585591
], dtype=np.float32))
586592
starts = None if starts_np is None else mx.array(starts_np)
587593
ends = None if ends_np is None else mx.array(ends_np)
588-
out = mx.full((2, 2), -123, dtype=mx.int32)
589594

590-
with pytest.raises(TopKPathCDirectError, match="limited to unmasked rows"):
591-
topk_selector_tilelang_direct(scores, k=2, starts=starts, ends=ends, out=out)
592-
mx.eval(out)
593-
assert np.asarray(out).tolist() == [[-123, -123], [-123, -123]]
595+
out_c = _topk_tilelang_direct_output(scores, k=2, starts=starts, ends=ends)
596+
out_ref = topk_selector_reference(scores, k=2, starts=starts, ends=ends)
597+
mx.eval(out_c, out_ref)
598+
599+
assert _to_index_sets(out_c) == _to_index_sets(out_ref)
594600

595601

596602
def test_path_c_keeps_expected_exact_order_for_ties() -> None:
@@ -610,28 +616,28 @@ def test_path_c_keeps_expected_exact_order_for_ties() -> None:
610616
assert np.asarray(out_c).tolist() == expected
611617

612618

613-
def test_path_c_direct_rejects_masked_ties_and_sentinels() -> None:
619+
def test_path_c_direct_masked_ties_and_sentinels_parity() -> None:
620+
"""Masked rows with ties and short intervals (``-1`` sentinels) reach
621+
set-equality parity with the reference through the owner-output Path C
622+
prim. Tie-exact sequence ordering is not asserted (set membership is the
623+
contract), but the short-interval sentinel set must contain ``-1``."""
624+
614625
scores = mx.array(np.array([
615626
[9.0, 7.0, 7.0, 6.0, 7.0, 5.0, 7.0, 4.0],
616627
[1.0, 8.0, 8.0, 8.0, 3.0, 8.0, 2.0, 8.0],
617628
[0.0, 9.0, 8.0, 7.0, 6.0, 5.0, 4.0, 4.0],
618629
], dtype=np.float32))
619630
starts = mx.array(np.array([1, 2, 6], dtype=np.int32))
620631
ends = mx.array(np.array([7, 6, 8], dtype=np.int32))
621-
out = mx.full((3, 4), -123, dtype=mx.int32)
622632

623-
with pytest.raises(TopKPathCDirectError, match="limited to unmasked rows"):
624-
topk_selector_tilelang_direct(scores, k=4, starts=starts, ends=ends, out=out)
633+
out_c = _topk_tilelang_direct_output(scores, k=4, starts=starts, ends=ends)
625634
out_ref = topk_selector_reference(scores, k=4, starts=starts, ends=ends)
626-
mx.eval(out, out_ref)
635+
mx.eval(out_c, out_ref)
627636

628-
expected = [
629-
[1, 2, 4, 6],
630-
[2, 3, 5, 4],
631-
[6, 7, -1, -1],
632-
]
633-
assert np.asarray(out_ref).tolist() == expected
634-
assert np.asarray(out).tolist() == [[-123] * 4] * 3
637+
assert _to_index_sets(out_c) == _to_index_sets(out_ref)
638+
# Row 2's interval [6, 8) has only 2 valid columns for k=4, so the
639+
# reference pads with the -1 sentinel; Path C must do the same.
640+
assert -1 in _to_index_sets(out_c)[2]
635641

636642

637643
@pytest.mark.parametrize("dtype", [mx.float32, mx.float16])
@@ -728,28 +734,58 @@ def fail_path_b(*args: object, **kwargs: object) -> None:
728734
assert _to_index_sets(out) == _to_index_sets(out_ref)
729735

730736

731-
def test_public_entry_point_auto_uses_reference_for_masked_calls(
737+
def test_public_entry_point_auto_uses_path_c_for_masked_calls(
732738
monkeypatch: pytest.MonkeyPatch,
733739
) -> None:
740+
"""Masked AUTO calls now route through the owner-output Path C prim
741+
(passing starts/ends through), not the pure-MLX reference."""
742+
734743
scores = mx.array(np.arange(64, dtype=np.float32).reshape(1, 64))
735744
starts = mx.array(np.array([8], dtype=np.int32))
736745
ends = mx.array(np.array([48], dtype=np.int32))
746+
sentinel = mx.array(np.array([[47, 46, 45, 44, 43, 42, 41, 40]], dtype=np.int32))
747+
seen: dict[str, object] = {}
737748

738-
def fail_path_c(*args: object, **kwargs: object) -> None:
739-
raise AssertionError("masked backend='auto' must not call unmasked Path C direct")
749+
def path_c_direct(scores_arg, k, *, starts=None, ends=None, out=None):
750+
seen["starts"] = starts
751+
seen["ends"] = ends
752+
return sentinel
740753

741754
def fail_path_b(*args: object, **kwargs: object) -> None:
742755
raise AssertionError("backend='auto' must not call retired Path B")
743756

744757
monkeypatch.setattr(
745758
"cppmega_mlx.nn._tilelang.topk_selector.topk_selector_tilelang_direct",
746-
fail_path_c,
759+
path_c_direct,
747760
)
748761
monkeypatch.setattr(
749762
"cppmega_mlx.nn._tilelang.topk_selector.topk_selector_metal",
750763
fail_path_b,
751764
)
752765
out = topk_selector(scores, k=8, starts=starts, ends=ends, backend="auto")
766+
assert seen["starts"] is starts
767+
assert seen["ends"] is ends
768+
assert np.asarray(out).tolist() == [[47, 46, 45, 44, 43, 42, 41, 40]]
769+
770+
771+
def test_public_entry_point_auto_masked_falls_back_to_reference_when_path_c_fails(
772+
monkeypatch: pytest.MonkeyPatch,
773+
) -> None:
774+
"""If the masked Path C prim cannot dispatch, AUTO still falls back to the
775+
pure-MLX reference rather than raising."""
776+
777+
scores = mx.array(np.arange(64, dtype=np.float32).reshape(1, 64))
778+
starts = mx.array(np.array([8], dtype=np.int32))
779+
ends = mx.array(np.array([48], dtype=np.int32))
780+
781+
def fail_path_c(*args: object, **kwargs: object) -> None:
782+
raise TopKPathCDirectError("masked compile failed")
783+
784+
monkeypatch.setattr(
785+
"cppmega_mlx.nn._tilelang.topk_selector.topk_selector_tilelang_direct",
786+
fail_path_c,
787+
)
788+
out = topk_selector(scores, k=8, starts=starts, ends=ends, backend="auto")
753789
out_ref = topk_selector_reference(scores, k=8, starts=starts, ends=ends)
754790
assert _to_index_sets(out) == _to_index_sets(out_ref)
755791

0 commit comments

Comments
 (0)